On Mon, 3 Jul 2023, Richard Sandiford wrote:
> Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > When trying to associate (v + INT_MAX) + INT_MAX we are using
> > the TREE_OVERFLOW bit to check for correctness. That isn't
> > working for VECTOR_CSTs and it can't in general when one considers
> > VL vectors. It looks like it should work for COMPLEX_CSTs but
> > I didn't try to single out _Complex int in this change.
> >
> > The following makes sure that for vectors we use the fallback of
> > using unsigned arithmetic when associating the above to
> > v + (INT_MAX + INT_MAX).
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?
> >
> > Thanks,
> > Richard.
> >
> > PR middle-end/110495
> > * tree.h (TREE_OVERFLOW): Do not mention VECTOR_CSTs
> > since we do not set TREE_OVERFLOW on those since the
> > introduction of VL vectors.
> > * match.pd (x +- CST +- CST): For VECTOR_CST do not look
> > at TREE_OVERFLOW to determine validity of association.
> >
> > * gcc.dg/tree-ssa/addadd-2.c: Amend.
> > * gcc.dg/tree-ssa/forwprop-27.c: Adjust.
> > ---
> > gcc/match.pd | 9 +++++----
> > gcc/testsuite/gcc.dg/tree-ssa/addadd-2.c | 1 +
> > gcc/testsuite/gcc.dg/tree-ssa/forwprop-27.c | 4 +++-
> > gcc/tree.h | 2 +-
> > 4 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/gcc/match.pd b/gcc/match.pd
> > index f09583bbcac..d193a572005 100644
> > --- a/gcc/match.pd
> > +++ b/gcc/match.pd
> > @@ -3025,7 +3025,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> > (with { tree cst = const_binop (outer_op == inner_op
> > ? PLUS_EXPR : MINUS_EXPR,
> > type, @1, @2); }
> > - (if (cst && !TREE_OVERFLOW (cst))
> > + (if (INTEGRAL_TYPE_P (type) && cst && !TREE_OVERFLOW (cst))
> > (inner_op @0 { cst; } )
> > /* X+INT_MAX+1 is X-INT_MIN. */
> > (if (INTEGRAL_TYPE_P (type) && cst
> > @@ -3037,7 +3037,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> > (view_convert (inner_op
> > (view_convert:utype @0)
> > (view_convert:utype
> > - { drop_tree_overflow (cst); }))))))))))))))
> > + { TREE_OVERFLOW (cst)
> > + ? drop_tree_overflow (cst) : cst; }))))))))))))))
>
> It looks like the whole ?(with ?)? expects cst to be nonnull,
> but the ?last resort? doesn't check it (unless I'm misreading).
> Would it be easier to add a top-level ?if (cst)?? (Obviously
> a preexisting thing.)
Hmm, indeed. I've added an outer if (cst).
> >
> > /* (CST1 - A) +- CST2 -> CST3 - A */
> > (for outer_op (plus minus)
> > @@ -3049,7 +3050,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> > forever if something doesn't simplify into a constant. */
> > (if (!CONSTANT_CLASS_P (@0))
> > (minus (outer_op! (view_convert @1) @2) (view_convert @0)))
> > - (if (!ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0))
> > + (if (!INTEGRAL_TYPE_P (TREE_TYPE (@0))
> > || TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0)))
> > (view_convert (minus (outer_op! @1 (view_convert @2)) @0))
> > (if (types_match (type, @0) && !TYPE_OVERFLOW_SANITIZED (type))
> > @@ -3068,7 +3069,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> > forever if something doesn't simplify into a constant. */
> > (if (!CONSTANT_CLASS_P (@0))
> > (plus (view_convert @0) (minus! @1 (view_convert @2))))
> > - (if (!ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0))
> > + (if (!INTEGRAL_TYPE_P (TREE_TYPE (@0))
> > || TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0)))
> > (view_convert (plus @0 (minus! (view_convert @1) @2)))
> > (if (types_match (type, @0) && !TYPE_OVERFLOW_SANITIZED (type))
>
> I didn't understand this part. Doesn't it mean that we allow
> overflow-inducing reassociations for all vector integer types,
> albeit not immediately folded away?
Oh, indeed - I though I can circumvent the TREE_OVERFLOW check for
those (where I don't yet have a testcase) by altering the guarding
check - but that check is to guard the TYPE_OVERFLOW_* checks.
To fix this we'd have to add unsigned fallbacks like for the above
pattern. I'm going to remove the two hunks for now.
> Also, why do we keep the:
>
> !ANY_INTEGRAL_TYPE_P (type) || TYPE_OVERFLOW_WRAPS (type)
>
> in the outer ifs?
I think this is distict types since both patterns have conditiona
conversions in the patterns they match. Otherwise it would be
redundant checking and would have been better if placed as 'else'
branch of the inner ifs.
As said, going to fix the missing conditional on non-null 'cst'
and drop the two hunks unrelated to the PR (which also were
wrong - thanks for noticing).
Richard.
>
> But that's just me not understanding match.pd very well.
> Feel free to ignore if it's nonsense. :)
>
> Thanks,
> Richard
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
