Re: [PATCH] Fix artificial overflow during GENERIC folding

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

On Wed, May 24, 2023 at 11:56 AM Eric Botcazou via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
>
> on the attached testcase, the Ada compiler gives a bogus warning:
> storage_offset1.ads:16:52: warning: Constraint_Error will be raised at run
> time [enabled by default]
>
> This directly comes from the GENERIC folding setting a bogus TREE_OVERFLOW on
> an INTEGER_CST during the (T)P - (T)(P + A) -> -(T) A transformation:
>
>   /* (T)P - (T)(P + A) -> -(T) A */
>   (simplify
>    (minus (convert? @0)
>     (convert (plus:c @@0 @1)))
>    (if (INTEGRAL_TYPE_P (type)
>         && TYPE_OVERFLOW_UNDEFINED (type)
>         && element_precision (type) <= element_precision (TREE_TYPE (@1)))
>     (with { tree utype = unsigned_type_for (type); }
>      (convert (negate (convert:utype @1))))
>     (if (element_precision (type) <= element_precision (TREE_TYPE (@1))
>          /* For integer types, if A has a smaller type
>             than T the result depends on the possible
>             overflow in P + A.
>             E.g. T=size_t, A=(unsigned)429497295, P>0.
>             However, if an overflow in P + A would cause
>             undefined behavior, we can assume that there
>             is no overflow.  */
>          || (INTEGRAL_TYPE_P (TREE_TYPE (@1))
>              && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@1))))
>      (negate (convert @1)))))
>   (simplify
>    (minus (convert @0)
>     (convert (pointer_plus @@0 @1)))
>    (if (INTEGRAL_TYPE_P (type)
>         && TYPE_OVERFLOW_UNDEFINED (type)
>         && element_precision (type) <= element_precision (TREE_TYPE (@1)))
>     (with { tree utype = unsigned_type_for (type); }
>      (convert (negate (convert:utype @1))))
>     (if (element_precision (type) <= element_precision (TREE_TYPE (@1))
>          /* For pointer types, if the conversion of A to the
>             final type requires a sign- or zero-extension,
>             then we have to punt - it is not defined which
>             one is correct.  */
>          || (POINTER_TYPE_P (TREE_TYPE (@0))
>              && TREE_CODE (@1) == INTEGER_CST
>              && tree_int_cst_sign_bit (@1) == 0))
>      (negate (convert @1)))))
>
> Ironically enough, this occurs because of the intermediate conversion to an
> unsigned type which is supposed to hide overflows, but is counter-productive
> for constants because TREE_OVERFLOW is always set for them, so it ends up
> setting a bogus TREE_OVERFLOW when converting back to the original type.
>
> The fix simply redirects INTEGER_CSTs to the other, direct path without the
> intermediate conversion to the unsigned type.
>
> Tested on x86-64/Linux, OK for the mainline?

Hmm.  gimple_resimplifyN do

  if (constant_for_folding (res_op->ops[0]))
    {
      tree tem = NULL_TREE;
      if (res_op->code.is_tree_code ())
        {
          auto code = tree_code (res_op->code);
          if (IS_EXPR_CODE_CLASS (TREE_CODE_CLASS (code))
              && TREE_CODE_LENGTH (code) == 1)
            tem = const_unop (code, res_op->type, res_op->ops[0]);
        }
      else
        tem = fold_const_call (combined_fn (res_op->code), res_op->type,
                               res_op->ops[0]);
      if (tem != NULL_TREE
          && CONSTANT_CLASS_P (tem))
        {
          if (TREE_OVERFLOW_P (tem))
            tem = drop_tree_overflow (tem);
          res_op->set_value (tem);

so why doesn't that apply here?  Ah, because it's for GENERIC folding
and there we use fold_buildN.

I don't like littering the patterns with this and it's likely far from the
only cases we have?  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).

I'm also hesitant to invent another syntax, like

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

that would then code-generate a if (TREE_OVERFLOW_P (..)) drop_tree_overflow ().

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?

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?
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.

Richard.

>
> 2023-05-24  Eric Botcazou <ebotcazou@adacore.com>
>
>         * match.pd ((T)P - (T)(P + A) -> -(T) A): Avoid artificial overflow
>         on constants.
>
>
> 2023-05-24  Eric Botcazou <ebotcazou@adacore.com>
>
>         * gnat.dg/specs/storage_offset1.ads: New test.
>
> --
> Eric Botcazou