Re: [PATCH] tree-optimization/110243 - kill off IVOPTs split_offset

[Richard Biener <rguenther@suse.de>]

On Wed, 21 Jun 2023, Richard Biener wrote:

> On Tue, 20 Jun 2023, Richard Sandiford wrote:
> 
> > Richard Biener <rguenther@suse.de> writes:
> > > On Mon, 19 Jun 2023, Richard Sandiford wrote:
> > >
> > >> Jeff Law <jeffreyalaw@gmail.com> writes:
> > >> > On 6/16/23 06:34, Richard Biener via Gcc-patches wrote:
> > >> >> IVOPTs has strip_offset which suffers from the same issues regarding
> > >> >> integer overflow that split_constant_offset did but the latter was
> > >> >> fixed quite some time ago.  The following implements strip_offset
> > >> >> in terms of split_constant_offset, removing the redundant and
> > >> >> incorrect implementation.
> > >> >> 
> > >> >> The implementations are not exactly the same, strip_offset relies
> > >> >> on ptrdiff_tree_p to fend off too large offsets while split_constant_offset
> > >> >> simply assumes those do not happen and truncates them.  By
> > >> >> the same means strip_offset also handles POLY_INT_CSTs but
> > >> >> split_constant_offset does not.  Massaging the latter to
> > >> >> behave like strip_offset in those cases might be the way to go?
> > >> >> 
> > >> >> Bootstrapped and tested on x86_64-unknown-linux-gnu.
> > >> >> 
> > >> >> Comments?
> > >> >> 
> > >> >> Thanks,
> > >> >> Richard.
> > >> >> 
> > >> >> 	PR tree-optimization/110243
> > >> >> 	* tree-ssa-loop-ivopts.cc (strip_offset_1): Remove.
> > >> >> 	(strip_offset): Make it a wrapper around split_constant_offset.
> > >> >> 
> > >> >> 	* gcc.dg/torture/pr110243.c: New testcase.
> > >> > Your call -- IMHO you know this code far better than I.
> > >> 
> > >> +1, but LGTM FWIW.  I couldn't see anything obvious (and valid)
> > >> that split_offset_1 handles and split_constant_offset doesn't.
> > >
> > > I think it's only the INTEGER_CST vs. ptrdiff_tree_p where the
> > > latter (used in split_offset_1) handles POLY_INT_CSTs.  split_offset
> > > also computes the offset in poly_int64 and checks it fits
> > > (to some extent) while split_constant_offset simply converts all
> > > INTEGER_CSTs to ssizetype because it knows it starts from addresses
> > > only.
> > >
> > > An alternative fix would have been to rewrite signed arithmetic
> > > to unsigned in strip_offset_1.
> > >
> > > I wonder if we want to change split_constant_offset to record the
> > > offset in a poly_int64 and have a wrapper converting it back to
> > > a tree for data-ref analysis.
> > 
> > Sounds a good idea if it's easily doable.
> > 
> > > Then we can at least put cst_and_fits_in_hwi checks in the code?
> > 
> > What would they be protecting against, if we're dealing with
> > address arithmetic?
> 
> While tree-data-ref.cc deals with address arithmetic only IVOPTs
> at least also runs it on general IVs, for example for uses
> in the exit condition.
> 
> Adding the following to strip_offset
> 
>   gcc_assert (POINTER_TYPE_P (TREE_TYPE (expr))
>               || (INTEGRAL_TYPE_P (TREE_TYPE (expr))
>                   && TYPE_PRECISION (TREE_TYPE (expr)) <= TYPE_PRECISION 
> (sizetype)));
> 
> runs into ICEs when testing a 32bit target.
> 
> But IVOPTs only makes use of the computed offset when it strips
> it off address uses.  But what I only now realized is that
> IVOPTs strip_offset is also used by loop distribution.
> 
> I'm going to split the patch in two at least to make these things
> more obvious before changing the implementation.

Hmm, but still split_constant_offset for example does

    case MULT_EXPR:
      if (TREE_CODE (op1) != INTEGER_CST)
        return false;

      split_constant_offset (op0, &var0, &off0, &op0_range, cache, limit);
      op1_range.set (TREE_TYPE (op1), wi::to_wide (op1), wi::to_wide 
(op1));
      *off = size_binop (MULT_EXPR, off0, fold_convert (ssizetype, op1));
      if (!compute_distributive_range (type, op0_range, code, op1_range,
                                       off, result_range))
        return false;
      *var = fold_build2 (MULT_EXPR, sizetype, var0,
                          fold_convert (sizetype, op1));

so *var is affected as well since we might truncate op1 here.  In
fact we at the end do

  if (INTEGRAL_TYPE_P (type))
    *var = fold_convert (sizetype, *var);

so truncate things (the API is documented to do that).

The issue in the PR the change is fixing is that we end up with
an expression that overflows but uses signed arithmetic and so
we miscompile it later.  IIRC the fixes to split_constant_offset
always were that the sum of the base + offset wasn't equal to
the original expression, right?

Richard.