Richard Biener <rguent...@suse.de> writes:
> 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 ;)
Sounds good.
> I'll see to do that as followup if you think that makes sense.
Yeah, sure, definitely not a reason to hold up this patch.
Thanks,
Richard
> 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
>>
