[PATCH] Fix PR83418

[PATCH] Fix PR83418

[Richard Biener]

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.

Index: gcc/vr-values.c
===================================================================
--- gcc/vr-values.c	(revision 255622)
+++ gcc/vr-values.c	(working copy)
@@ -445,11 +445,12 @@ vr_values::extract_range_for_var_from_co
   tree  min, max, type;
   value_range *limit_vr;
   type = TREE_TYPE (var);
-  gcc_assert (limit != var);
 
   /* For pointer arithmetic, we only keep track of pointer equality
-     and inequality.  */
-  if (POINTER_TYPE_P (type) && cond_code != NE_EXPR && cond_code != EQ_EXPR)
+     and inequality.  If we arrive here with unfolded conditions like
+     _1 > _1 do not derive anything.  */
+  if ((POINTER_TYPE_P (type) && cond_code != NE_EXPR && cond_code != EQ_EXPR)
+      || limit == var)
     {
       set_value_range_to_varying (vr_p);
       return;
Index: gcc/testsuite/gcc.dg/torture/pr83418.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr83418.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/torture/pr83418.c	(working copy)
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+
+void
+yj (int j4)
+{
+  int t3;
+
+  for (t3 = 0; t3 < 6; ++t3)
+    {
+      short int v4 = t3;
+
+      if (v4 == j4 || v4 > t3)
+	for (;;)
+	  {
+	  }
+    }
+}

Re: [PATCH] Fix PR83418

[Jeff Law]

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.

Jeff

Re: [PATCH] Fix PR83418

[Richard Biener]

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. 

Richard. 

>Jeff

Re: [PATCH] Fix PR83418

[Richard Biener]

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);

Re: [PATCH] Fix PR83418

[Jeff Law]

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.

jeff

Re: [PATCH] Fix PR83418

[Richard Biener]

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. 

Richard. 

>jeff

Re: [PATCH] Fix PR83418

[Jeff Law]

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.  I'll add your patch to
one of my build/test cycles.

Jeff
> 
> Richard. 
> 
>> jeff
> 

Re: [PATCH] Fix PR83418

[Jeff Law]

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. 
So I went ahead with a bootstrap and regression test with this patch.
If (of course) worked fine.  I'm installing it on the trunk.

jeff

Re: [PATCH] Fix PR83418

[Richard Biener]

On December 21, 2017 5:40:48 AM GMT+01:00, Jeff Law <law@redhat.com> 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. 
>So I went ahead with a bootstrap and regression test with this patch.
>If (of course) worked fine.  I'm installing it on the trunk.

Thanks. 

Richard. 

>jeff

Re: [PATCH] Fix PR83418

[Richard Biener]

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)

Richard.

Re: [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