Re: [PATCH] Fix artificial overflow during GENERIC folding

[Richard Biener <richard.guenther@gmail.com>]

On Wed, May 24, 2023 at 2:39 PM Eric Botcazou <botcazou@adacore.com> wrote:
>
> > I don't like littering the patterns with this and it's likely far from the
> > only cases we have?
>
> Maybe, but that's the only problematic case we have in Ada.  It occurs only on
> mainline because we have streamlined address calculations there, from out-of-
> line to inline expansion, i.e. from run time to compile time.
>
> > Since we did move some of the patterns from fold-const.cc to match.pd and
> > the frontends might be interested in TREE_OVERFLOW (otherwise we'd just
> > scrap that!) I'm not sure removing the flag is good (and I never was really
> > convinced the setting for the implementation defined behavior on conversion
> > to unsigned is good).
>
> Yes, the Ada front-end relies on the TREE_OVERFLOW flag to detect overflows at
> compile time, so it cannot be removed, but it must be set correctly, which is
> not the case here: (T)p - (T) (p + 4) where T is signed should just yield -4.
>
> > Am I correct that the user writing such a conversion in Ada _should_
> > get a constraint violation?  So it's just the middle-end introducing it
> > to avoid undefined signed overflow that's on error?
>
> Yes, it's a Constraint_Error in Ada to convert a value of an unsigned type to
> a signed type if it does not fit in the signed type.
>
> > I'll also note that fold_convert_const_int_from_int shouldn't set
> > TREE_OVERFLOW on unsigned destination types?  So it's the
> > outer conversion back to signed that generates the TREE_OVERFLOW?
>
> Yes, 4 is converted to unsigned, then negated, yielding a huge number, and the
> final conversion back to signed yields -4 with TREE_OVERFLOW set.
>
> > Would it help to use a (view_convert ...) here?  For non-constants that
> > should be folded back to a sign changing (convert ...) but the constant
> > folding should hopefully happen earlier?  But it's again implementation
> > defined behavior we have here, so not sure we need TREE_OVERFLOW at all.
>
> I'm not sure we need to jump through too many hoops here: the intermediate
> conversion trick is a kludge because we lack a proper method to selectively
> disable undefined overflow at run time, but that's not the case at compile
> time where we have a finer-grained control (and even different rules) so I
> don't really see a problem with handling the two cases differently.

But nobody is going to understand why the INTEGER_CST case goes the
other way.  As you say we don't have a good way to say we're doing
this to avoid undefined behavior, but then a view-convert back would
be a good way to indicate that?  I can't come up with a better name
for a custom operator we could also use,

  (convert_without_overflow (negate (convert:utype @1))))

maybe?  As said, if view_convert works I prefer that.  Does it?

Richard.

>
> --
> Eric Botcazou
>
>