On Thu, Apr 28, 2022 at 8:43 PM Jeff Law <jeffreya...@gmail.com> wrote:
>
>
>
> On 4/28/2022 10:30 AM, Aldy Hernandez wrote:
> > [Jeff, this is the same patch I sent you last week with minor tweaks
> > to the commit message.]
> And I just dropped it into the tester. We should have the vast majority
> of targets tested by this time tomorrow.
>
> >
> > [Despite the verbosity of the message, this is actually a pretty
> > straightforward patch. It should've gone in last cycle, but there
> > was a nagging regression I couldn't get to until after stage1
> > had closed.]
> You had other, non-work/gcc priorities. No worries at missing gcc-12.
Ha. I keep forgetting I have a permanent excuse going forward for any
delays... parenthood!
>
>
> >
> > There are 3 uses of EVRP in DOM that must be converted.
> > Unfortunately, they need to be converted in one go, so further
> > splitting of this patch would be problematic.
> ACK
>
>
> > There's nothing here earth shattering. It's all pretty obvious in
> > retrospect, but I've added a short description of each use to aid in
> > reviewing:
> >
> > * Convert evrp use in cprop to ranger.
> >
> > This is easy, as cprop in DOM was converted to the ranger API last
> > cycle, so this is just a matter of using a ranger instead of an
> > evrp_range_analyzer.
> Yea. We may have covered this before, but what does ranger do with a
> conditional equivalence?
>
> ie if we have something like
>
> if (a == b)
> {
> blah blah
> }
>
> Within the true arm we know there's an equivalence. *But* we don't want
> to just blindly replace a with b or vice-versa. The equivalence is
> primarily useful for simplification rather than propagation.
>
> In fact, we every much do not want to cprop in these cases. It rarely
> helps in a meaningful way and there's no good way to know which is the
> better value to use. Canonicalization on SSA_NAMEs doesn't really help
> due to SSA_NAME recycling.
Ranger only returns constants, not symbolics, so the call to
range_of_expr is guaranteed to be constant. I verified that this
subtle change to range_of_expr didn't cause any problems in my
conversion to the value_query API in commit e1d01f4973 (last cyce):
There is one subtle change. The call to vr_value's
op_with_constant_singleton_value_range can theoretically return
non-constants, unlike the range_query API which only returns constants.
In this particular case it doesn't matter because the symbolic stuff will
have been handled by the const_and_copies/avail_exprs read in the
SSA_NAME_VALUE copy immediately before. I have verified this is the case
by asserting that all calls to op_with_constant_singleton_value_range at
this point return either NULL or an INTEGER_CST.
Just to be clear, even though ranger doesn't return a symbolic, it
does know about relations via the relation oracle the ranger uses. So
on the TRUE arm, it will know that a==b, which it can leverage to fold
any statements. This could be done with the statement folding API in
gimple-range-fold.h (or via range_of_stmt). You could theoretically
query the relation oracle yourself with query_relation() which are
exported by the ranger (via the range_query class it inherits from).
But for the case above, we're calling range_of_expr which is
guaranteed to be a constant.
Since we're calling range_of_expr on operands, we shouldn't be folding
anything. OTOH, if you were to call range_of_stmt on the conditional,
it could return TRUE/FALSE based on how it would fold. In
dom_opt_dom_walker::fold_cond() which I have provided in the patch, we
will fold conditionals with knowledge of relations, because it uses
range_of_stmt.
BTW, when frange and prange come live, there will be other attributes
that may be attached to a range. For instance for prange, we'll have
a "points-to" attribute. For example:
p_5 = &foo;
The range of p_5 will be nonzero with a points-to attribute of &foo +
0 or some such.
So later this cycle we'll have symbolics of sorts, but never in the
range itself, but as an attribute say prange::get_points_to().
Similarly for floats. We'll probably have not-NAN, not-INF, etc attributes.
>
>
> >
> > * Convert evrp use in threader to ranger.
> >
> > The idea here is to use the hybrid approach we used for the initial
> > VRP threader conversion last cycle. The DOM threader will continue
> > using the forward threader infrastructure while continuing to query
> > DOM data structures, and only if the conditional does not relsolve,
> > using the ranger. This gives us the best of both worlds, and is a
> > proven approach.
> >
> > Furthermore, as frange and prange come live in the next cycle, we
> > can move away from the forward threader altogether, and just add
> > another backward threader. This will not only remove the last use
> > of the forward threader, but will allow us to remove at least 1 or 2
> > threader instances.
> Excellent.
>
> >
> > * Convert conditional folding to use the method used by the ranger and
> > evrp. Previously DOM was calling into the guts of
> > simplify_using_ranges::vrp_visit_cond_stmt. The blessed way now is
> > using fold_cond() which rewrites the conditional and edges
> > automatically.
> >
> > When legacy is removed, simplify_using_ranges will be further
> > cleaned up, and there will only be one entry point into simplifying
> > a statement.
> Sure.
>
> >
> > * DOM was setting global ranges determined from unreachable edges as a
> > side-effect of using the evrp engine. We must handle these cases
> > before nuking evrp, and DOM seems like a good fit. I've just moved
> > the snippet to DOM, but it could live anywhere else we do a DOM
> > walk.
> It was a semi-desirable to record/refine global ranges in DOM, but I'd
> be surprised if too much code depended on that. Presumably there's a
> testcase in the suite which we don't handle well if we don't let DOM
> refine/record global ranges?
>
> >
> > For the record, this is the case *vrp handled:
> >
> > <bb C>:
> > ...
> > if (c_5(D) != 5)
> > goto <bb N>;
> > else
> > goto <bb M>;
> > <bb N>:
> > __builtin_unreachable ();
> > <bb M>:
> >
> > If M dominates all uses of c_5, we can set the global range of c_5
> > to [5,5].
> And that will only happen if M eventually loops back to the conditional,
> right? Otherwise all the uses wouldn't be dominated. I suspect this is
> exceedingly rare in practice an it looks really familiar. This is
> coming from a testcase, right?
>
> This is the only part of the patch that makes me go "ewwww", so I would
> like to at least explore if we can rethink the value of that test.
[the above is discussed down thread]
>
> > There is a a regression on Wstringop-overflow-4.C which I'm planning
> > on XFAILing. It's another variant of the usual middle-end false
> > positives: having no ranges produces no warnings, but slightly refined
> > ranges, or worse-- isolating specific problematic cases in the
> > threader causes flare-ups.
> ACK.
>
>
> >
> > As an aside, as Richi has suggested, I think we should discuss
> > restricting the threader's ability to thread highly unlikely paths.
> > These cause no end of pain for middle-end warnings. However,
> > I don't know if this would conflict with path isolation for
> > things like null dereferencing. ISTR you were interested in this.
> I've never though too much about it. You're thinking of Richi :-)
>
> It's a balance. I bet if we stop threading those paths we're going to
> see other issues start popping up like uninitialized warnings.
>
> It may be the case that it's really the constant propagation on an
> isolated path that's more problematical for those warnings. But I would
> probably contend that what isolation is doing is showing how certain
> constant values can flow into places where they're not expected and
> could cause problems. So I wouldn't necessary suggest throttling it.
>
> Happy to be proven wrong here!
>
>
> >
> > BTW, I think the Wstringop-overflow-4.C test is problematic and I've
> > attached my analysis. Basically the regression is caused by a bad
> > interaction with the rounding/alignment that placement new has inlined
> > into the IL. This happens for int16_r[] which the test is testing.
> > Ranger can glean some range info, which causes DOM threading to
> > isolate a path which causes a warning.
> Presumably this is triggering a warning at the new() call?
Yes.
Aldy
