[patch] Fix tree-optimization/91169

[Richard Biener]

On Fri, Aug 2, 2019 at 10:24 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Thu, Aug 1, 2019 at 7:11 PM Eric Botcazou <ebotcazou@adacore.com> wrote:
> >
> > > So isn't this an issue with the code before when we have a RANGE_EXPR
> > > that covers the wrapping point?
> >
> > No, see the reduced testcase attached in the PR.
> >
> > > Then index > max_index and may not catch the found element with
> > >
> > >     /* Do we have match?  */
> > >     if (wi::cmpu (access_index, index) >= 0
> > >         && wi::cmpu (access_index, max_index) <= 0)
> > >       return cval;
> > >
> > > ?  We seem to be careful to convert the indices to the index type but
> > > above use unsigned compares - isn't that the underlying issue?
> >
> > Not really, everything is properly unsigned at this point, so the traversal
> > wraps around in an unsigned type.
> >
> > > I'm not sure how "doing from scratch" would fix things - we simply rely
> > > on reliably detecting an element match.
> > >
> > > I wonder how much we can rely on the array domain type to be in sync
> > > with the constructor field entries.
> >
> > At least we need to take into account the wraparound.
> >
> > > Also we're using offset_int here - doesn't that break when with
> > > Ada we have an array type with domain [uint128_t_max - 1, uint128_t_max + 2]
> > > in a type larger than int128_t?  Or, since we're interpreting it
> > > inconsistently signed/unsigned, a 128bit integer typed domain wraps
> > > around the sign?
> >
> > Integer types are limited to 64 bits in GNAT.
> >
> > > If that's not an issue then using signed compare unconditionally is
> > > a valid fix as well I guess.
> >
> > But all the types are unsigned here so this doesn't seem appealing.
> >
> > > But in reality 'index' should never really wrap, that is, if the domain
> > > is of 'int' type and starts with INT_MAX then the array shall not have
> > > more than 1 element.  No?
> >
> > The range is [-1, 1] converted to sizetype, which is unsigned, so the low
> > bound is UINT32_MAX and the high bound is 1.
>
> Oh.  OK, now I see.  Still, if we have this domain and a CONSTRUCTOR
> with a RANGE_EXPR UINT32_MAX, 1 and we are asking for access_index
> zero then the old code did
>
>     /* Do we have match?  */
>     if (wi::cmpu (access_index, index) >= 0
>         && wi::cmpu (access_index, max_index) <= 0)
>       return cval;
>
> and index == UINT32_MAX, max_index == 1.  So we don't match it
> but return NULL, assuming the constructor value is zero?
>
> Not sure if the Ada FE ever creates RANGE_EXPRs for CONSTRUCTOR
> entries, for single-valued entries the check of course works.  But as it
> was written above I assumed certain ordering conditions must hold...
> (grepping shows no traces of RANGE_EXPR in gigi)
>
> I'm hoping for a GCC C extension to specify array domains so the
> GIMPLE FE can be used to play with these kind of things...
>
> > > The patch you posted isn't a real fix btw, since iff the break was
> > > hit we either missed an index that is present or we found a gap
> > > but then we can break.
> >
> > No, we cannot break if the index wraps around, that's the bug.
> >
> > > So I'd appreciate more explanation on how the index "wraps".
> >
> > See above.  I can rewind the entire chain of events if need be, but this is a
> > long one starting a long time ago with the initial blunder with PLUS_EXPR and
> > sizetype.
>
> Yeah.  Note that you can very well use signed TYPE_DOMAIN nowadays
> (OK, no guarantees...).
>
> I think we should kludge around similar to how we do in stor-layout.c
> which does
>
>                 /* ???  When it is obvious that the range is signed
>                    represent it using ssizetype.  */
>                 if (TREE_CODE (lb) == INTEGER_CST
>                     && TREE_CODE (ub) == INTEGER_CST
>                     && TYPE_UNSIGNED (TREE_TYPE (lb))
>                     && tree_int_cst_lt (ub, lb))
>                   {
>                     lb = wide_int_to_tree (ssizetype,
>                                            offset_int::from (wi::to_wide (lb),
>                                                              SIGNED));
>                     ub = wide_int_to_tree (ssizetype,
>                                            offset_int::from (wi::to_wide (ub),
>                                                              SIGNED));
>                   }
>
> So I am testing the attached.

Testing went OK but it looks like acats doesn't honor
RUNTESTFLAGS so I got no multilib testing for it :/
And the PR didn't contain sth I could plug into gnat.dg so I checked
with visual inspection of dumps on the reduced testcase.
I notice pretty-printing is also confused about the wrapping ;)

  static struct p__intarray___PAD intarray = {.F={-100, [0]=0, 100}};

the [0]= is not necessary.

Anyway, I can see bogus IL before and still after the proposed patch :/
This is because I forgot to adjust cfield handling of setting index.

So I'm testing the adjusted patch attached which fixes the IL
and for testing have patched gcc/testsuite/ada/acats/run_all.sh
to add -m32.

Richard.

> Richard.
>
> >
> > --
> > Eric Botcazou
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-pr91169
Type: application/octet-stream
Size: 2869 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20190802/2c4e7879/attachment.obj>