[PATCH] Fix PR83418

[Jeff Law]

On 01/02/2018 02:26 AM, Richard Biener wrote:
> On Tue, 19 Dec 2017, Jeff Law wrote:
> 
>> On 12/15/2017 09:30 AM, Richard Biener wrote:
>>> On December 15, 2017 5:27:14 PM GMT+01:00, Jeff Law <law@redhat.com> wrote:
>>>> On 12/15/2017 01:10 AM, Richard Biener wrote:
>>>>> On Thu, 14 Dec 2017, Richard Biener wrote:
>>>>>
>>>>>> On December 14, 2017 4:43:42 PM GMT+01:00, Jeff Law <law@redhat.com>
>>>> wrote:
>>>>>>> On 12/14/2017 01:54 AM, Richard Biener wrote:
>>>>>>>>
>>>>>>>> IVOPTs (at least) leaves unfolded stmts in the IL and VRP
>>>>>>> overzealously
>>>>>>>> asserts they cannot happen.
>>>>>>>>
>>>>>>>> Bootstrap and regtest running on x86_64-unknown-linux-gnu.
>>>>>>>>
>>>>>>>> Richard.
>>>>>>>>
>>>>>>>> 2017-12-14  Richard Biener  <rguenther@suse.de>
>>>>>>>>
>>>>>>>> 	PR tree-optimization/83418
>>>>>>>> 	* vr-values.c
>>>>>>> (vr_values::extract_range_for_var_from_comparison_expr):
>>>>>>>> 	Instead of asserting we don't get unfolded comparisons deal with
>>>>>>>> 	them.
>>>>>>>>
>>>>>>>> 	* gcc.dg/torture/pr83418.c: New testcase.
>>>>>>> I think this also potentially affects dumping.  I've seen the
>>>> dumper
>>>>>>> crash trying to access a INTEGER_CST where we expected to find an
>>>>>>> SSA_NAME while iterating over a statement's operands.
>>>>>>>
>>>>>>> I haven't submitted the workaround because I hadn't tracked down
>>>> the
>>>>>>> root cause to verify something deeper isn't wrong.
>>>>>>
>>>>>> Yes, I've seen this as well, see my comment in the PR. The issue is
>>>> that DOM calls VRP analyze (and dump) routines with not up to date
>>>> operands during optimize_stmt. 
>>>>>
>>>>> I had the following in my tree to allow dumping.
>>>>>
>>>>> Richard.
>>>>>
>>>>> Index: gcc/tree-ssa-dom.c
>>>>> ===================================================================
>>>>> --- gcc/tree-ssa-dom.c  (revision 255640)
>>>>> +++ gcc/tree-ssa-dom.c  (working copy)
>>>>> @@ -2017,6 +2017,7 @@ dom_opt_dom_walker::optimize_stmt (basic
>>>>>                  undefined behavior that get diagnosed if they're
>>>> left in 
>>>>> the
>>>>>                  IL because we've attached range information to new
>>>>>                  SSA_NAMES.  */
>>>>> +             update_stmt_if_modified (stmt);
>>>>>               edge taken_edge = NULL;
>>>>>               evrp_range_analyzer.vrp_visit_cond_stmt (as_a <gcond *>
>>>>
>>>>> (stmt),
>>>>>                                                        &taken_edge);
>>>>>
>>>> I think this implies something earlier changed a statement without
>>>> updating it.
>>>
>>> Dom itself does this and delays updating on purpose as an optimization. That doesn't work quite well when dispatching into different code. 
>> I honestly can't remember the history around not updating -- and I've
>> always found the explicit need to mark and update clunky.
>>
>> I'd rather have the IL be consistent -- it's hard to believe there's a
>> major benefit to deferring the operand update.
> 
> SSA operand updating is/was one of the major slownesses at some point
> so we started micro-optimizing it.
> 
> There's the odd modified bit in GIMPLE stmts which we neither consistently
> set when modifying operands in a way that require update_stmt nor
> do we avoid re-scanning the stmt when doing update_stmt (which
> just sets the modified bit and then calls update_stmt_if_modified).
> 
> IMHO we should either get rid of the modified bit or use / update it
> consistently.  But there's the general data structure issue of
> the immediate use list of SSA names and the SSA operand list of
> stmts where the former keys back to the latter but not the other
> way around - this means we can't update the stmt operand list without
> re-scanning the whole stmt when substituting SSA names for example
> (but we could set the modified bit there).
> 
> Note that update_stmt fixes both the stmt operand list and also
> immediate use data in case somebody changed a stmt via the
> set_rhs/lhs interfaces (which _also_ don't set the modified bit).
> 
> So it's somewhat of a mess but rather than trying to "fix" the
> modified bit thing I'd see to fix the data structures to allow
> O(1) updating of the stmt SSA operand list from places that use
> SET_USE (use_p, X) (propagate_value and friends).  Maybe we can
> get rid of the stmt SSA operand list and instead walk the
> operand slots (for single-rhs we'd need to linearly walk
> the GENERIC tree in a quite constrained way).  We've got rid of
> the DEFs list already after all...  (I know this was on Michas
> TODO list)
The core of the operand scanning pre-dates tuples -- so finding operands
was fairly painful as we had to walk the entire expression.  Thus the
operand cache was born.

As you mention, we ought to be able to look at the operand slots these
days which may change things enough that we fundamentally don't need the
cache anymore.

We'd probably need to special case expressions/references in the operand
slots (ie, MEM_REF, perhaps others).  But if done right we could
probably update the walkers to DTRT with the same API and drop the
modified bit nonsense entirely.

jeff