Re: [PATCH] shrink-wrap: Don't put on incoming EDGE_CROSSING [PR98289]
On Wed, Dec 16, 2020 at 06:31:35PM +0100, Jakub Jelinek wrote: > On Wed, Dec 16, 2020 at 11:17:29AM -0600, Segher Boessenkool wrote: > > It was just a bootstrap+regression check, so no new testcase was needed. > > I don't remember what target, but powerpc (32+64, BE) probably. > > I'll bootstrap/regtest the patch on powerpc64 and powerpc64le then. Thanks! > > > The testcase in the patch (which I've excended today compared to what > > > the patch had yesterday) has the cases of a single as well as multiple > > > edges to the ideal prologue bb in the cold partition, yet I can't > > > reproduce > > > the ICE. > > > As for what changed, I remember fixing PR87475 which is related to > > > redirection of crossing jumps. > > > > But that is for cfglayout mode? Shrink-wrap has to do everything in > > cfgrtl mode unfortunately, because of the partitioning stuff. Sounds > > like a good cancidate otherwise. > > The patch affects both modes. patch_jump_insn would in some cases ICE > and now it doesn't, instead returns false and > rtl_redirect_edge_and_branch_force > then can deal with it another way. But that can also ICE then, it's just pushing the problem around, unless the original test was wrong? In any case, if you see no problems anymore, I'll just try to stop worrying :-) Segher
Re: [PATCH] shrink-wrap: Don't put on incoming EDGE_CROSSING [PR98289]
On Wed, Dec 16, 2020 at 11:17:29AM -0600, Segher Boessenkool wrote: > It was just a bootstrap+regression check, so no new testcase was needed. > I don't remember what target, but powerpc (32+64, BE) probably. I'll bootstrap/regtest the patch on powerpc64 and powerpc64le then. > > > The testcase in the patch (which I've excended today compared to what > > the patch had yesterday) has the cases of a single as well as multiple > > edges to the ideal prologue bb in the cold partition, yet I can't reproduce > > the ICE. > > As for what changed, I remember fixing PR87475 which is related to > > redirection of crossing jumps. > > But that is for cfglayout mode? Shrink-wrap has to do everything in > cfgrtl mode unfortunately, because of the partitioning stuff. Sounds > like a good cancidate otherwise. The patch affects both modes. patch_jump_insn would in some cases ICE and now it doesn't, instead returns false and rtl_redirect_edge_and_branch_force then can deal with it another way. Jakub
Re: [PATCH] shrink-wrap: Don't put on incoming EDGE_CROSSING [PR98289]
On Wed, Dec 16, 2020 at 04:36:37PM +0100, Jakub Jelinek wrote: > On Wed, Dec 16, 2020 at 09:28:56AM -0600, Segher Boessenkool wrote: > > This used to not work, as mentioned in the original patch submission: > > https://patchwork.ozlabs.org/project/gcc/patch/52f14532eb742ac8d878a185a46a88da7b0326eb.1442588483.git.seg...@kernel.crashing.org/ > > I wonder what changed? > > There is no testcase in that thread I could find, on which it would ICE. It was just a bootstrap+regression check, so no new testcase was needed. I don't remember what target, but powerpc (32+64, BE) probably. > The testcase in the patch (which I've excended today compared to what > the patch had yesterday) has the cases of a single as well as multiple > edges to the ideal prologue bb in the cold partition, yet I can't reproduce > the ICE. > As for what changed, I remember fixing PR87475 which is related to > redirection of crossing jumps. But that is for cfglayout mode? Shrink-wrap has to do everything in cfgrtl mode unfortunately, because of the partitioning stuff. Sounds like a good cancidate otherwise. > > So, the situation is there are multiple incoming edges to where we want > > to place the prologue. Because we have to have one single edge to put > > the prologue on (that is how the infrastructure we have works, it is not > > anything fundamental), we create a new block that just jumps to the > > block that needs the prologue, use that new edge for where we place the > > prologue, and then redirect all edges to the first block to the new > > block. (We cannot in all case simply fall through.) > > I believe this case is covered in the testcase. Yeah... It is never certaint what it will look like in the generated machine code, but yup, probably. In either case, if regstrap now works with this on all targets, this is great :-) I just wish I knew what changed. > > > In the former case, they > > > are usually conditional jumps that patch_jump_insn can handle just fine, > > > after all, they were previously crossing and will be crossing after > > > the redirection too, just to a different label. And in the powerpc64 > > > case, it is a simple_jump instead that again seems to be handled by > > > patch_jump_insn just fine. > > > > So what changed in the last five years that makes redirecting > > EDGE_CROSSING jumps now always work? Does that also apply to > > EDGE_COMPLEX, btw? Knowing this would make at least me a lot less > > nervous about this patch :-) > > I think EDGE_COMPLEX generally can't be redirected, it is ok to punt on > those. Yeah, it is the kitchen sink "do not touch this" flag :-) Thanks again, Segher
Re: [PATCH] shrink-wrap: Don't put on incoming EDGE_CROSSING [PR98289]
On Wed, Dec 16, 2020 at 09:28:56AM -0600, Segher Boessenkool wrote: > This used to not work, as mentioned in the original patch submission: > https://patchwork.ozlabs.org/project/gcc/patch/52f14532eb742ac8d878a185a46a88da7b0326eb.1442588483.git.seg...@kernel.crashing.org/ > I wonder what changed? There is no testcase in that thread I could find, on which it would ICE. The testcase in the patch (which I've excended today compared to what the patch had yesterday) has the cases of a single as well as multiple edges to the ideal prologue bb in the cold partition, yet I can't reproduce the ICE. As for what changed, I remember fixing PR87475 which is related to redirection of crossing jumps. > So, the situation is there are multiple incoming edges to where we want > to place the prologue. Because we have to have one single edge to put > the prologue on (that is how the infrastructure we have works, it is not > anything fundamental), we create a new block that just jumps to the > block that needs the prologue, use that new edge for where we place the > prologue, and then redirect all edges to the first block to the new > block. (We cannot in all case simply fall through.) I believe this case is covered in the testcase. > > In the former case, they > > are usually conditional jumps that patch_jump_insn can handle just fine, > > after all, they were previously crossing and will be crossing after > > the redirection too, just to a different label. And in the powerpc64 > > case, it is a simple_jump instead that again seems to be handled by > > patch_jump_insn just fine. > > So what changed in the last five years that makes redirecting > EDGE_CROSSING jumps now always work? Does that also apply to > EDGE_COMPLEX, btw? Knowing this would make at least me a lot less > nervous about this patch :-) I think EDGE_COMPLEX generally can't be redirected, it is ok to punt on those. Jakub
Re: [PATCH] shrink-wrap: Don't put on incoming EDGE_CROSSING [PR98289]
Hi! On Wed, Dec 16, 2020 at 02:35:32PM +0100, Jakub Jelinek wrote: > As mentioned in the PR, shrink-wrapping disqualifies for prologue > placement basic blocks that have EDGE_CROSSING incoming edge. > I don't see why that is necessary, those edges seem to be redirected > just fine, both on x86_64 and powerpc64. This used to not work, as mentioned in the original patch submission: https://patchwork.ozlabs.org/project/gcc/patch/52f14532eb742ac8d878a185a46a88da7b0326eb.1442588483.git.seg...@kernel.crashing.org/ I wonder what changed? So, the situation is there are multiple incoming edges to where we want to place the prologue. Because we have to have one single edge to put the prologue on (that is how the infrastructure we have works, it is not anything fundamental), we create a new block that just jumps to the block that needs the prologue, use that new edge for where we place the prologue, and then redirect all edges to the first block to the new block. (We cannot in all case simply fall through.) > In the former case, they > are usually conditional jumps that patch_jump_insn can handle just fine, > after all, they were previously crossing and will be crossing after > the redirection too, just to a different label. And in the powerpc64 > case, it is a simple_jump instead that again seems to be handled by > patch_jump_insn just fine. So what changed in the last five years that makes redirecting EDGE_CROSSING jumps now always work? Does that also apply to EDGE_COMPLEX, btw? Knowing this would make at least me a lot less nervous about this patch :-) > Sure, redirecting an edge that was previously not crossing to be crossing or > vice versa can fail, but that is not what shrink-wrapping needs. It used to ICE when not disallowing EDGE_CROSSING. > Also tested in GCC 8 with this patch and don't see ICEs there either > (though, of course, I'm not suggesting we should backport this to release > branches). The patch looks fine to me of course, modulo that worry. Segher
[PATCH] shrink-wrap: Don't put on incoming EDGE_CROSSING [PR98289]
Hi!
As mentioned in the PR, shrink-wrapping disqualifies for prologue
placement basic blocks that have EDGE_CROSSING incoming edge.
I don't see why that is necessary, those edges seem to be redirected
just fine, both on x86_64 and powerpc64. In the former case, they
are usually conditional jumps that patch_jump_insn can handle just fine,
after all, they were previously crossing and will be crossing after
the redirection too, just to a different label. And in the powerpc64
case, it is a simple_jump instead that again seems to be handled by
patch_jump_insn just fine.
Sure, redirecting an edge that was previously not crossing to be crossing or
vice versa can fail, but that is not what shrink-wrapping needs.
Also tested in GCC 8 with this patch and don't see ICEs there either
(though, of course, I'm not suggesting we should backport this to release
branches).
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2020-12-16 Jakub Jelinek
PR rtl-optimization/98289
* shrink-wrap.c (can_get_prologue): Don't punt on EDGE_CROSSING
incoming edges.
* gcc.target/i386/pr98289.c: New test.
* gcc.dg/torture/pr98289.c: New test.
--- gcc/shrink-wrap.c.jj2020-07-28 15:39:09.983756571 +0200
+++ gcc/shrink-wrap.c 2020-12-15 19:15:00.213861334 +0100
@@ -494,7 +494,7 @@ can_get_prologue (basic_block pro, HARD_
edge e;
edge_iterator ei;
FOR_EACH_EDGE (e, ei, pro->preds)
-if (e->flags & (EDGE_COMPLEX | EDGE_CROSSING)
+if (e->flags & EDGE_COMPLEX
&& !dominated_by_p (CDI_DOMINATORS, e->src, pro))
return false;
--- gcc/testsuite/gcc.target/i386/pr98289.c.jj 2020-12-16 13:15:08.579847596
+0100
+++ gcc/testsuite/gcc.target/i386/pr98289.c 2020-12-16 14:26:16.863157527
+0100
@@ -0,0 +1,54 @@
+/* PR rtl-optimization/98289 */
+/* { dg-do compile { target { ! ia32 } } } */
+/* { dg-require-effective-target freorder } */
+/* { dg-options "-O2 -freorder-blocks-and-partition
-fdump-rtl-pro_and_epilogue-details" } */
+/* { dg-final { scan-rtl-dump-times "Performing shrink-wrapping" 4
"pro_and_epilogue" } } */
+
+int bar (void) __attribute__((cold));
+
+void
+foo (int x)
+{
+ if (x)
+__builtin_abort ();
+}
+
+void
+baz (int x)
+{
+ if (__builtin_expect (x, 0))
+{
+ bar ();
+ bar ();
+ bar ();
+}
+}
+
+void
+qux (int x, int y, int z, int w)
+{
+ if (x || y || z || w)
+__builtin_abort ();
+}
+
+int
+corge (int x, int y, int z, int w, int u)
+{
+ if (__builtin_expect (x, 0))
+goto lab;
+ u++;
+ if (__builtin_expect (y, 0))
+goto lab;
+ u *= 2;
+ if (__builtin_expect (z, 0))
+goto lab;
+ u |= 42;
+ if (__builtin_expect (w, 0))
+{
+ lab:;
+ bar ();
+ bar ();
+ if (bar () > 32) goto lab;
+}
+ return u;
+}
--- gcc/testsuite/gcc.dg/torture/pr98289.c.jj 2020-12-16 14:27:01.391659297
+0100
+++ gcc/testsuite/gcc.dg/torture/pr98289.c 2020-12-16 14:27:29.442345443
+0100
@@ -0,0 +1,52 @@
+/* PR rtl-optimization/98289 */
+/* { dg-do compile { target freorder } } */
+/* { dg-options "-O2 -freorder-blocks-and-partition" } */
+
+int bar (void) __attribute__((cold));
+
+void
+foo (int x)
+{
+ if (x)
+__builtin_abort ();
+}
+
+void
+baz (int x)
+{
+ if (__builtin_expect (x, 0))
+{
+ bar ();
+ bar ();
+ bar ();
+}
+}
+
+void
+qux (int x, int y, int z, int w)
+{
+ if (x || y || z || w)
+__builtin_abort ();
+}
+
+int
+corge (int x, int y, int z, int w, int u)
+{
+ if (__builtin_expect (x, 0))
+goto lab;
+ u++;
+ if (__builtin_expect (y, 0))
+goto lab;
+ u *= 2;
+ if (__builtin_expect (z, 0))
+goto lab;
+ u |= 42;
+ if (__builtin_expect (w, 0))
+{
+ lab:;
+ bar ();
+ bar ();
+ if (bar () > 32) goto lab;
+}
+ return u;
+}
Jakub
