Re: [PATCH][PR tree-optimization/67816] Fix jump threading when DOM removes conditionals in jump threading path

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Dec 2, 2015 at 11:56 PM, Jeff Law <law@redhat.com> wrote:
> On 12/02/2015 08:35 AM, Richard Biener wrote:
>
>>>> be possible to make it do that much like I extended SCCVN to do this
>>>> (when doing the DOM walk see if any incoming edge is marked executable
>>>> and if not, mark all outgoing edges as not executable, if the block is
>>>> executable
>>>> at the time we process the last stmt determine if we can compute the
>>>> edge
>>>> that ends up always executed and mark all others as not executable)
>>>
>>>
>>> Essentially yes. I'm using the not-executable flag and bypassing things
>>> when
>>> it's discovered.
>
> I took at look at what you did with SCCVN and I like it better than what I
> was doing -- your handling is more complete.
>
> There's some code that ought to be factored out.  In particular at the start
> of before_dom_children you've got code that clears EDGE_EXECUTABLE.
> There's no reason we should duplicate in SCCVN, DOM and perhaps other DOM
> walkers in the future.  If you've got a place in mind where you think it
> ought to live (cfg-something) speak up.  Else I'll find a spot on my own.

I think domwalk.[ch] itself might be a good enough spot, a static method
inside the domwalk class

  bool before_dom_children_track_and_query_bb_executable (basic_block);

using a derived class that automagically does this would still require
to explicitely call the parent function before_dom_children, so better
make it explicit.

And document it is non-optimistic thus all edges have to be marked
EDGE_EXECUTABLE before the domwalk and before_dom_children
is supposed to mark non-executable outgoing edges.

>>>
>>> The most interesting side effect, and one I haven't fully analyzed yet is
>>> an
>>> unexpected jump thread -- which I've traced back to differences in what
>>> the
>>> alias oracle is able to find when we walk unaliased vuses. Which makes
>>> totally no sense that it's unable to find the unaliased vuse in the
>>> simplified CFG, but finds it when we don't remove the unexecutable edge.
>>> As
>>> I said, it makes no sense to me yet and I'm still digging.
>>
>>
>> The walking of PHI nodes is quite simplistic to avoid doing too much work
>> so
>> an extra (not executable) edge may confuse it enough.  So this might be
>> "expected".  Adding a flag on whether EDGE_EXECUTABLE is to be
>> trusted would be an option (also helping SCCVN).
>
> It was actually the opposite effect.  ie, with the simplified CFG, we missed
> the jump thread, but with the more complex CFG we found the jump thread.
> And the edge we were removing didn't (at first glance) appear to be of any
> real significance.

Oh, I see...

> I'm not seeing the oddity anymore now that I've converted DOM to mimick what
> you were doing in SCCVN.  So I'm going to assume I botched something
> somewhere.
>
> Back to analysis to see if there's other fallout.
>
> jeff
>