Message ID | patch-18267-tamar@arm.com |
---|---|
State | New |
Headers | show |
Series | middle-end: inspect all exits for additional annotations for loop. | expand |
On Wed, 14 Feb 2024, Tamar Christina wrote: > Hi All, > > Attaching a pragma to a loop which has a complex condition often gets the pragma > dropped. e.g. > > #pragma GCC novector > while (i < N && parse_tables_n--) > > before lowering this is represented as: > > if (ANNOTATE_EXPR <i <= 305 && parse_tables_n-- != 0, no-vector>) ... > > But after lowering the condition is broken appart and attached to the final > component of the expression: > > if (parse_tables_n.2_2 != 0) goto <D.4456>; else goto <D.4453>; > <D.4456>: > iftmp.1D.4452 = 1; > goto <D.4454>; > <D.4453>: > iftmp.1D.4452 = 0; > <D.4454>: > D.4451 = .ANNOTATE (iftmp.1D.4452, 2, 0); > if (D.4451 != 0) goto <D.4442>; else goto <D.4440>; > <D.4440>: > > and it's never heard from again because during replace_loop_annotate we only > inspect the loop header and latch for annotations. > > Since annotations were supposed to apply to the loop as a whole this fixes it > by also checking the loop exit src blocks for annotations. > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Ok for master? I think this isn't entirely good. For simple cases for do {} while the condition ends up in the latch while for while () {} loops it ends up in the header. In your case the latch isn't empty so it doesn't end up with the conditional. I think your patch is OK to the point of looking at all loop exit sources but you should elide the special-casing of header and latch since it's really only exit conditionals that matter. Richard. > Thanks, > Tamar > > gcc/ChangeLog: > > * tree-cfg.cc (replace_loop_annotate): Inspect loop edges for annotations. > > gcc/testsuite/ChangeLog: > > * gcc.dg/vect/vect-novect_gcond.c: New test. > > --- inline copy of patch -- > diff --git a/gcc/testsuite/gcc.dg/vect/vect-novect_gcond.c b/gcc/testsuite/gcc.dg/vect/vect-novect_gcond.c > new file mode 100644 > index 0000000000000000000000000000000000000000..01e69cbef9d51b234c08a400c78dc078d53252f1 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/vect/vect-novect_gcond.c > @@ -0,0 +1,39 @@ > +/* { dg-add-options vect_early_break } */ > +/* { dg-require-effective-target vect_early_break_hw } */ > +/* { dg-require-effective-target vect_int } */ > +/* { dg-additional-options "-O3" } */ > + > +/* { dg-final { scan-tree-dump-not "LOOP VECTORIZED" "vect" } } */ > + > +#include "tree-vect.h" > + > +#define N 306 > +#define NEEDLE 136 > + > +int table[N]; > + > +__attribute__ ((noipa)) > +int foo (int i, unsigned short parse_tables_n) > +{ > + parse_tables_n >>= 9; > + parse_tables_n += 11; > +#pragma GCC novector > + while (i < N && parse_tables_n--) > + table[i++] = 0; > + > + return table[NEEDLE]; > +} > + > +int main () > +{ > + check_vect (); > + > +#pragma GCC novector > + for (int j = 0; j < N; j++) > + table[j] = -1; > + > + if (foo (0, 0xFFFF) != 0) > + __builtin_abort (); > + > + return 0; > +} > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > index cdd439fe7506e7bc33654ffa027b493f23d278ac..a29681bffb902d2d05e3f18764ab519aacb3c5bc 100644 > --- a/gcc/tree-cfg.cc > +++ b/gcc/tree-cfg.cc > @@ -327,6 +327,10 @@ replace_loop_annotate (void) > if (loop->latch) > replace_loop_annotate_in_block (loop->latch, loop); > > + /* Then also check all other exits. */ > + for (auto e : get_loop_exit_edges (loop)) > + replace_loop_annotate_in_block (e->src, loop); > + > /* Push the global flag_finite_loops state down to individual loops. */ > loop->finite_p = flag_finite_loops; > } > > > > >
> > I think this isn't entirely good. For simple cases for > do {} while the condition ends up in the latch while for while () {} > loops it ends up in the header. In your case the latch isn't empty > so it doesn't end up with the conditional. > > I think your patch is OK to the point of looking at all loop exit > sources but you should elide the special-casing of header and > latch since it's really only exit conditionals that matter. > That makes sense, since in both cases the edges are in the respective blocks. Should have thought about it more. So how about this one. Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. Ok for master? Thanks, Tamar gcc/ChangeLog: * tree-cfg.cc (replace_loop_annotate): Inspect loop edges for annotations. gcc/testsuite/ChangeLog: * gcc.dg/vect/vect-novect_gcond.c: New test. --- inline copy of patch --- diff --git a/gcc/testsuite/gcc.dg/vect/vect-novect_gcond.c b/gcc/testsuite/gcc.dg/vect/vect-novect_gcond.c new file mode 100644 index 0000000000000000000000000000000000000000..01e69cbef9d51b234c08a400c78dc078d53252f1 --- /dev/null +++ b/gcc/testsuite/gcc.dg/vect/vect-novect_gcond.c @@ -0,0 +1,39 @@ +/* { dg-add-options vect_early_break } */ +/* { dg-require-effective-target vect_early_break_hw } */ +/* { dg-require-effective-target vect_int } */ +/* { dg-additional-options "-O3" } */ + +/* { dg-final { scan-tree-dump-not "LOOP VECTORIZED" "vect" } } */ + +#include "tree-vect.h" + +#define N 306 +#define NEEDLE 136 + +int table[N]; + +__attribute__ ((noipa)) +int foo (int i, unsigned short parse_tables_n) +{ + parse_tables_n >>= 9; + parse_tables_n += 11; +#pragma GCC novector + while (i < N && parse_tables_n--) + table[i++] = 0; + + return table[NEEDLE]; +} + +int main () +{ + check_vect (); + +#pragma GCC novector + for (int j = 0; j < N; j++) + table[j] = -1; + + if (foo (0, 0xFFFF) != 0) + __builtin_abort (); + + return 0; +} diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc index cdd439fe7506e7bc33654ffa027b493f23d278ac..bdffc3b4ed277724e81b7dd67fe7966e8ece0c13 100644 --- a/gcc/tree-cfg.cc +++ b/gcc/tree-cfg.cc @@ -320,12 +320,9 @@ replace_loop_annotate (void) for (auto loop : loops_list (cfun, 0)) { - /* First look into the header. */ - replace_loop_annotate_in_block (loop->header, loop); - - /* Then look into the latch, if any. */ - if (loop->latch) - replace_loop_annotate_in_block (loop->latch, loop); + /* Check all exit source blocks for annotations. */ + for (auto e : get_loop_exit_edges (loop)) + replace_loop_annotate_in_block (e->src, loop); /* Push the global flag_finite_loops state down to individual loops. */ loop->finite_p = flag_finite_loops;
> Am 14.02.2024 um 16:16 schrieb Tamar Christina <tamar.christina@arm.com>: > > >> >> >> I think this isn't entirely good. For simple cases for >> do {} while the condition ends up in the latch while for while () {} >> loops it ends up in the header. In your case the latch isn't empty >> so it doesn't end up with the conditional. >> >> I think your patch is OK to the point of looking at all loop exit >> sources but you should elide the special-casing of header and >> latch since it's really only exit conditionals that matter. >> > > That makes sense, since in both cases the edges are in the respective > blocks. Should have thought about it more. > > So how about this one. > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Ok for master? Ok Richard > Thanks, > Tamar > > gcc/ChangeLog: > > * tree-cfg.cc (replace_loop_annotate): Inspect loop edges for annotations. > > gcc/testsuite/ChangeLog: > > * gcc.dg/vect/vect-novect_gcond.c: New test. > > --- inline copy of patch --- > > diff --git a/gcc/testsuite/gcc.dg/vect/vect-novect_gcond.c b/gcc/testsuite/gcc.dg/vect/vect-novect_gcond.c > new file mode 100644 > index 0000000000000000000000000000000000000000..01e69cbef9d51b234c08a400c78dc078d53252f1 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/vect/vect-novect_gcond.c > @@ -0,0 +1,39 @@ > +/* { dg-add-options vect_early_break } */ > +/* { dg-require-effective-target vect_early_break_hw } */ > +/* { dg-require-effective-target vect_int } */ > +/* { dg-additional-options "-O3" } */ > + > +/* { dg-final { scan-tree-dump-not "LOOP VECTORIZED" "vect" } } */ > + > +#include "tree-vect.h" > + > +#define N 306 > +#define NEEDLE 136 > + > +int table[N]; > + > +__attribute__ ((noipa)) > +int foo (int i, unsigned short parse_tables_n) > +{ > + parse_tables_n >>= 9; > + parse_tables_n += 11; > +#pragma GCC novector > + while (i < N && parse_tables_n--) > + table[i++] = 0; > + > + return table[NEEDLE]; > +} > + > +int main () > +{ > + check_vect (); > + > +#pragma GCC novector > + for (int j = 0; j < N; j++) > + table[j] = -1; > + > + if (foo (0, 0xFFFF) != 0) > + __builtin_abort (); > + > + return 0; > +} > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > index cdd439fe7506e7bc33654ffa027b493f23d278ac..bdffc3b4ed277724e81b7dd67fe7966e8ece0c13 100644 > --- a/gcc/tree-cfg.cc > +++ b/gcc/tree-cfg.cc > @@ -320,12 +320,9 @@ replace_loop_annotate (void) > > for (auto loop : loops_list (cfun, 0)) > { > - /* First look into the header. */ > - replace_loop_annotate_in_block (loop->header, loop); > - > - /* Then look into the latch, if any. */ > - if (loop->latch) > - replace_loop_annotate_in_block (loop->latch, loop); > + /* Check all exit source blocks for annotations. */ > + for (auto e : get_loop_exit_edges (loop)) > + replace_loop_annotate_in_block (e->src, loop); > > /* Push the global flag_finite_loops state down to individual loops. */ > loop->finite_p = flag_finite_loops; > <rb18267.patch>
--- /dev/null +++ b/gcc/testsuite/gcc.dg/vect/vect-novect_gcond.c @@ -0,0 +1,39 @@ +/* { dg-add-options vect_early_break } */ +/* { dg-require-effective-target vect_early_break_hw } */ +/* { dg-require-effective-target vect_int } */ +/* { dg-additional-options "-O3" } */ + +/* { dg-final { scan-tree-dump-not "LOOP VECTORIZED" "vect" } } */ + +#include "tree-vect.h" + +#define N 306 +#define NEEDLE 136 + +int table[N]; + +__attribute__ ((noipa)) +int foo (int i, unsigned short parse_tables_n) +{ + parse_tables_n >>= 9; + parse_tables_n += 11; +#pragma GCC novector + while (i < N && parse_tables_n--) + table[i++] = 0; + + return table[NEEDLE]; +} + +int main () +{ + check_vect (); + +#pragma GCC novector + for (int j = 0; j < N; j++) + table[j] = -1; + + if (foo (0, 0xFFFF) != 0) + __builtin_abort (); + + return 0; +} diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc index cdd439fe7506e7bc33654ffa027b493f23d278ac..a29681bffb902d2d05e3f18764ab519aacb3c5bc 100644 --- a/gcc/tree-cfg.cc +++ b/gcc/tree-cfg.cc @@ -327,6 +327,10 @@ replace_loop_annotate (void) if (loop->latch) replace_loop_annotate_in_block (loop->latch, loop); + /* Then also check all other exits. */ + for (auto e : get_loop_exit_edges (loop)) + replace_loop_annotate_in_block (e->src, loop); + /* Push the global flag_finite_loops state down to individual loops. */ loop->finite_p = flag_finite_loops; }