On Wed, May 29, 2019 at 11:28 PM Marc Glisse <marc.gli...@inria.fr> 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).
>
> 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?
It works for me. Any pass ordering changes should probably be done
as followup anyways since they might expose other issues (as you've seen...)
Richard.
> > I also see that the Og pass pipeline misses the second walloca pass
> > completely (and also the warn_restrict pass).
> >
> > Given code sinkings obvious effects on SSA value-range representation
> > it may make sense to add another instance of that pass earlier.
>
> --
> Marc Glisse
