On Wed, 13 Oct 2021, Richard Sandiford wrote: > Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > > The following makes sure to rewrite arithmetic with undefined behavior > > on overflow to a well-defined variant when moving them to be always > > executed as part of doing if-conversion for loop vectorization. > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu. > > > > Any comments? > > LGTM FWIW, although… > > > Thanks, > > Richard. > > > > 2021-10-11 Richard Biener <rguent...@suse.de> > > > > PR tree-optimization/102659 > > * tree-if-conv.c (need_to_rewrite_undefined): New flag. > > (if_convertible_gimple_assign_stmt_p): Mark the loop for > > rewrite when stmts with undefined behavior on integer > > overflow appear. > > (combine_blocks): Predicate also when we need to rewrite stmts. > > (predicate_statements): Rewrite affected stmts to something > > with well-defined behavior on overflow. > > (tree_if_conversion): Initialize need_to_rewrite_undefined. > > > > * gcc.dg/torture/pr69760.c: Adjust the testcase. > > * gcc.target/i386/avx2-vect-mask-store-move1.c: Expect to move > > the conversions to unsigned as well. > > --- > > gcc/testsuite/gcc.dg/torture/pr69760.c | 3 +- > > .../i386/avx2-vect-mask-store-move1.c | 2 +- > > gcc/tree-if-conv.c | 28 ++++++++++++++++++- > > 3 files changed, 29 insertions(+), 4 deletions(-) > > > > diff --git a/gcc/testsuite/gcc.dg/torture/pr69760.c > > b/gcc/testsuite/gcc.dg/torture/pr69760.c > > index 53733c7c6a4..47e01ae59bd 100644 > > --- a/gcc/testsuite/gcc.dg/torture/pr69760.c > > +++ b/gcc/testsuite/gcc.dg/torture/pr69760.c > > @@ -1,11 +1,10 @@ > > /* PR tree-optimization/69760 */ > > /* { dg-do run { target { { *-*-linux* *-*-gnu* *-*-uclinux* } && mmap } } > > } */ > > -/* { dg-options "-O2" } */ > > > > #include <unistd.h> > > #include <sys/mman.h> > > > > -__attribute__((noinline, noclone)) void > > +__attribute__((noinline, noclone)) static void > > test_func (double *a, int L, int m, int n, int N) > > { > > int i, k; > > diff --git a/gcc/testsuite/gcc.target/i386/avx2-vect-mask-store-move1.c > > b/gcc/testsuite/gcc.target/i386/avx2-vect-mask-store-move1.c > > index 989ba402e0e..6a47a09c835 100644 > > --- a/gcc/testsuite/gcc.target/i386/avx2-vect-mask-store-move1.c > > +++ b/gcc/testsuite/gcc.target/i386/avx2-vect-mask-store-move1.c > > @@ -78,4 +78,4 @@ avx2_test (void) > > abort (); > > } > > > > -/* { dg-final { scan-tree-dump-times "Move stmt to created bb" 6 "vect" } > > } */ > > +/* { dg-final { scan-tree-dump-times "Move stmt to created bb" 10 "vect" } > > } */ > > diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c > > index d7b7b309309..6a67acfeaae 100644 > > --- a/gcc/tree-if-conv.c > > +++ b/gcc/tree-if-conv.c > > @@ -132,6 +132,11 @@ along with GCC; see the file COPYING3. If not see > > predicate_statements for the kinds of predication we support. */ > > static bool need_to_predicate; > > > > +/* True if we have to rewrite stmts that may invoke undefined behavior > > + when a condition C was false so it doesn't if it is always executed. > > + See predicate_statements for the kinds of predication we support. */ > > +static bool need_to_rewrite_undefined; > > + > > /* Indicate if there are any complicated PHIs that need to be handled in > > if-conversion. Complicated PHI has more than two arguments and can't > > be degenerated to two arguments PHI. See more information in comment > > @@ -1042,6 +1047,12 @@ if_convertible_gimple_assign_stmt_p (gimple *stmt, > > fprintf (dump_file, "tree could trap...\n"); > > return false; > > } > > + else if (INTEGRAL_TYPE_P (TREE_TYPE (lhs)) > > + && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (lhs)) > > + && arith_code_with_undefined_signed_overflow > > + (gimple_assign_rhs_code (stmt))) > > + /* We have to rewrite stmts with undefined overflow. */ > > + need_to_rewrite_undefined = true; > > > > /* When if-converting stores force versioning, likewise if we > > ended up generating store data races. */ > > @@ -2563,6 +2574,20 @@ predicate_statements (loop_p loop) > > > > gsi_replace (&gsi, new_stmt, true); > > } > > + else if (INTEGRAL_TYPE_P (TREE_TYPE (gimple_assign_lhs (stmt))) > > + && TYPE_OVERFLOW_UNDEFINED > > + (TREE_TYPE (gimple_assign_lhs (stmt))) > > + && arith_code_with_undefined_signed_overflow > > + (gimple_assign_rhs_code (stmt))) > > + { > > + gsi_remove (&gsi, true); > > + gsi_insert_seq_before (&gsi, rewrite_to_defined_overflow (stmt), > > + GSI_SAME_STMT); > > + if (gsi_end_p (gsi)) > > + gsi = gsi_last_bb (gimple_bb (stmt)); > > + else > > + gsi_prev (&gsi); > > …perhaps there should be a GSI_* for this idiom.
Yeah, the issue is that gsi_remove might put gsi to one after the last stmt (gsi_end_p) which gsi_insert_seq_before happily handles (insert at the end of the block) but gsi_prev chokes on. I wonder what GSI_CONTINUE_LINKING would do with gsi_insert[_seq]_before. Hmm, it puts us at the first stmt inserted which might be OK in this case but the intention was to put us as the last. So we could eventually add GSI_LAST_NEW_STMT or so ;) I'll see to do that as followup if you think that makes sense. Note we currently document enum gsi_iterator_update { GSI_NEW_STMT, /* Only valid when single statement is added, move iterator to it. */ GSI_SAME_STMT, /* Leave the iterator at the same statement. */ GSI_CONTINUE_LINKING /* Move iterator to whatever position is suitable for linking other statements in the same direction. */ }; where the "Only valid when single statement" part is a bit odd. It seems to consistently point at the earliest inserted stmt (I guess first/last is a bit confusing and maybe earlierst/latest is better?) Richard. > Thanks, > Richard > > > + } > > else if (gimple_vdef (stmt)) > > { > > tree lhs = gimple_assign_lhs (stmt); > > @@ -2647,7 +2672,7 @@ combine_blocks (class loop *loop) > > insert_gimplified_predicates (loop); > > predicate_all_scalar_phis (loop); > > > > - if (need_to_predicate) > > + if (need_to_predicate || need_to_rewrite_undefined) > > predicate_statements (loop); > > > > /* Merge basic blocks. */ > > @@ -3148,6 +3173,7 @@ tree_if_conversion (class loop *loop, vec<gimple *> > > *preds) > > rloop = NULL; > > ifc_bbs = NULL; > > need_to_predicate = false; > > + need_to_rewrite_undefined = false; > > any_complicated_phi = false; > > > > /* Apply more aggressive if-conversion when loop or its outer loop were > -- Richard Biener <rguent...@suse.de> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)