[PATCH] vrp_prop: Use dom_walker for -Warray-bounds (PR tree-optimization/83312)

[Jeff Law]

On 12/13/2017 09:02 AM, David Malcolm wrote:
> On Wed, 2017-12-13 at 08:46 -0700, Jeff Law wrote:
>> On 12/13/2017 03:06 AM, Richard Biener wrote:
>>> On December 12, 2017 9:50:38 PM GMT+01:00, David Malcolm <dmalcolm@
>>> redhat.com> wrote:
>>>> PR tree-optimization/83312 reports a false positive from
>>>> -Warray-bounds.
>>>> The root cause is that VRP both:
>>>>
>>>> (a) updates a GIMPLE_COND to be always false, and
>>>>
>>>> (b) updates an ARRAY_REF in the now-unreachable other path to use
>>>> an
>>>>    ASSERT_EXPR with a negative index:
>>>>      def_stmt j_6 = ASSERT_EXPR <j_9, j_9 < 0>;
>>>>
>>>> When vrp_prop::check_all_array_refs () runs, the CFG hasn't yet
>>>> been
>>>> updated to take account of (a), and so a false positive is
>>>> emitted
>>>> when (b) is encountered.
>>>>
>>>> This patch fixes the false warning by converting
>>>>  vrp_prop::check_all_array_refs
>>>> from a simple iteration over all BBs to use a new dom_walker
>>>> subclass,
>>>> using the "skip_unreachable_blocks = true" mechanism to avoid
>>>> analyzing
>>>> (b).
>>>>
>>>> There didn't seem to be a pre-existing way to determine the
>>>> unique
>>>> out-edge after a GIMPLE_COND (if it has a constant cond), so I
>>>> added
>>>> a new gimple_cond_get_unique_successor_edge function.  Similarly,
>>>> something similar may apply for switches, so I put in a
>>>> gimple_get_unique_successor_edge (though I wasn't able to create
>>>> a
>>>> reproducer that used a switch).
>>>>
>>>> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
>>>>
>>>> OK for trunk?
>>>
>>> I don't like the GIMPLE.c bits a lot. Can't you use the existing
>>> known taken edge helper (too lazy to look up from my phone...)
>>> basically splitting out some code from cond processing in evrp for
>>> example? 
>>
>> I'm not sure those bits are needed at all. I think the right things
>> will
>> happen if we clear EDGE_EXECUTABLE on the appropriate edge in
>> vrp_folder::fold_predicate_in.
>>
>> That should cause the block in question to become unreachable (zero
>> preds).  So later when we do the domwal dom_walker::walk will see the
>> block as unreachable -- which avoids walking into it and also
>> triggers
>> the call to propagate_unreachable_to_edges which marks the outgoing
>> edges as not executable.
> 
> AIUI, dom_walker::bb_reachable only honors the EDGE_EXECUTABLE flags if
> m_skip_unreachable_blocks is set on the dom_walker.
> 
> However, dom_walker's ctor clears all of the EDGE_EXECUTABLE if that
> bool is set.
Ugh.  How unfortunate, though I understand why it would be written that way.

> 
> So, as written any edge flags that are touched in
> vrp_folder::fold_predicate_in will get reset when the dom walker is
> created.
> 
> So should the dom walker get created before the vrp_folder runs?
That would probably work, but something doesn't feel right about it.

Alternately we could to the dom_walker ctor that an initial state of
EDGE_EXECUTABLE is already set.

Jeff