I've never been too happy with the too large due to cast warnings. For that
matter, it seems like a lot of the unbounded alloca warning variants were
artifacts of the way we couldn't get precise ranges after vrp asserts had
disappeared and we were trying to guess at what the actual range in the
original code was. It's fragile at best.
I haven't been paying too much attention to walloca because the ranger gets
considerably better context ranges in the ranger walloca version, and we
are getting correct warnings for a variety of things we couldn't before. So
I was hoping to ignore this until we all agreed on what range, vrp etc will
look like going forward.
That being said, I could take a closer look at this xfail on Monday if
y'all would like. But I don't currently have strong opinions either way. I
guess it'll all change in the next few months.
Aldy
On Thu, May 30, 2019, 18:26 Jeff Law <l...@redhat.com> wrote:
> On 5/29/19 3:27 PM, Marc Glisse wrote:
> > On Mon, 20 May 2019, Richard Biener wrote:
> >
> >> On Mon, May 20, 2019 at 10:16 AM Marc Glisse <marc.gli...@inria.fr>
> >> wrote:
> >>>
> >>> On Mon, 20 May 2019, Richard Biener wrote:
> >>>
> >>>> On Sun, May 19, 2019 at 6:16 PM Marc Glisse <marc.gli...@inria.fr>
> >>>> wrote:
> >>>>>
> >>>>> Hello,
> >>>>>
> >>>>> 2 pieces:
> >>>>>
> >>>>> - the first one handles the case where the denominator is negative.
> It
> >>>>> doesn't happen often with exact_div, so I don't handle it
> >>>>> everywhere, but
> >>>>> this one looked trivial
> >>>>>
> >>>>> - handle the case where a pointer difference is cast to an unsigned
> >>>>> type
> >>>>> before being compared to a constant (I hit this in std::vector).
> >>>>> With some
> >>>>> range info we could probably handle some non-constant cases as
> well...
> >>>>>
> >>>>> The second piece breaks Walloca-13.c (-Walloca-larger-than=100 -O2)
> >>>>>
> >>>>> void f (void*);
> >>>>> void g (int *p, int *q)
> >>>>> {
> >>>>> __SIZE_TYPE__ n = (__SIZE_TYPE__)(p - q);
> >>>>> if (n < 100)
> >>>>> f (__builtin_alloca (n));
> >>>>> }
> >>>>>
> >>>>> At the time of walloca2, we have
> >>>>>
> >>>>> _1 = p_5(D) - q_6(D);
> >>>>> # RANGE [-2305843009213693952, 2305843009213693951]
> >>>>> _2 = _1 /[ex] 4;
> >>>>> # RANGE ~[2305843009213693952, 16140901064495857663]
> >>>>> n_7 = (long unsigned intD.10) _2;
> >>>>> _11 = (long unsigned intD.10) _1;
> >>>>> if (_11 <= 396)
> >>>>> [...]
> >>>>> _3 = allocaD.1059 (n_7);
> >>>>>
> >>>>> and warn.
> >>>>
> >>>> That's indeed to complicated relation of _11 to n_7 for
> >>>> VRP predicate discovery.
> >>>>
> >>>>> However, DOM3 later produces
> >>>>>
> >>>>> _1 = p_5(D) - q_6(D);
> >>>>> _11 = (long unsigned intD.10) _1;
> >>>>> if (_11 <= 396)
> >>>>
> >>>> while _11 vs. _1 works fine.
> >>>>
> >>>>> [...]
> >>>>> # RANGE [0, 99] NONZERO 127
> >>>>> _2 = _1 /[ex] 4;
> >>>>> # RANGE [0, 99] NONZERO 127
> >>>>> n_7 = (long unsigned intD.10) _2;
> >>>>> _3 = allocaD.1059 (n_7);
> >>>>>
> >>>>> so I am tempted to say that the walloca2 pass is too early, xfail the
> >>>>> testcase and file an issue...
> >>>>
> >>>> Hmm, there's a DOM pass before walloca2 already and moving
> >>>> walloca2 after loop opts doesn't look like the best thing to do?
> >>>> I suppose it's not DOM but sinking that does the important transform
> >>>> here? That is,
> >>>>
> >>>> Index: gcc/passes.def
> >>>> ===================================================================
> >>>> --- gcc/passes.def (revision 271395)
> >>>> +++ gcc/passes.def (working copy)
> >>>> @@ -241,9 +241,9 @@ along with GCC; see the file COPYING3.
> >>>> NEXT_PASS (pass_optimize_bswap);
> >>>> NEXT_PASS (pass_laddress);
> >>>> NEXT_PASS (pass_lim);
> >>>> - NEXT_PASS (pass_walloca, false);
> >>>> NEXT_PASS (pass_pre);
> >>>> NEXT_PASS (pass_sink_code);
> >>>> + NEXT_PASS (pass_walloca, false);
> >>>> NEXT_PASS (pass_sancov);
> >>>> NEXT_PASS (pass_asan);
> >>>> NEXT_PASS (pass_tsan);
> >>>>
> >>>> fixes it?
> >>>
> >>> I will check, but I don't think walloca uses any kind of on-demand
> >>> VRP, so
> >>> we still need some pass to update the ranges after sinking, which
> >>> doesn't
> >>> seem to happen until the next DOM pass.
> >>
> >> Oh, ok... Aldy, why's this a separate pass anyways? I think similar
> >> other warnigns are emitted from RTL expansion? So maybe we can
> >> indeed move the pass towards warn_restrict or late_warn_uninit.
> >
> > I tried moving it after 'sink' and that didn't help. Moving it next to
> > warn_restrict works for this test but breaks 2 others that currently
> > "work" by accident (+ one where the message changes between "unbounded"
> > and "too large", it isn't clear what the difference is between those
> > messages).
> So "unbounded" means there was no controlling predicate that would allow
> an upper bound on the size of the alloca to be determined.
>
> "too large" breaks down into two cases. One where we know it's too
> large, the other when it may be too large. The latter can happen due to
> casts and perhaps other things (controlling predicate with a dynamic
> upper bound maybe?)
>
>
> >
> > My suggestion, in addition to the original patch, is
> >
> > * gcc.dg/Walloca-13.c: Xfail.
> >
> > --- Walloca-13.c (revision 271742)
> > +++ Walloca-13.c (working copy)
> > @@ -1,12 +1,12 @@
> > /* { dg-do compile } */
> > /* { dg-require-effective-target alloca } */
> > /* { dg-options "-Walloca-larger-than=100 -O2" } */
> >
> > void f (void*);
> >
> > void g (int *p, int *q)
> > {
> > __SIZE_TYPE__ n = (__SIZE_TYPE__)(p - q);
> > if (n < 100)
> > - f (__builtin_alloca (n));
> > + f (__builtin_alloca (n)); // { dg-bogus "may be too large due to
> > conversion" "" { xfail { *-*-* } } }
> > }
> >
> > Is that ok?
> Aldy would need to chime in -- at first glance it looks like we've lost
> precision here.
>
> jeff
>
>
