On December 13, 2017 5:18:16 PM GMT+01:00, Jeff Law <l...@redhat.com> wrote: >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®rtested 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.
I'm quite sure that wouldn't help for VRP. I think David's approach is fine just we don't need any other API to get at a known executable outgoing edge. We can improve the existing one or just add the trivial folding required. Richard. >Jeff
