[RFA,PATCH] Bail in bounds_of_var_in_loop if no step found.

[Aldy Hernandez]

On 10/14/20 4:31 PM, Richard Biener wrote:
> On Wed, Oct 14, 2020 at 4:19 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>>
>>
>> On 10/14/20 9:43 AM, Richard Biener wrote:
>>> On Tue, Oct 13, 2020 at 6:12 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>>>
>>>>
>>>>
>>>> On 10/13/20 6:02 PM, Richard Biener wrote:
>>>>> On October 13, 2020 5:17:48 PM GMT+02:00, Aldy Hernandez via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>>>>> [Neither Andrew nor I are familiar with the SCEV code.  We treat it as
>>>>>> a
>>>>>> black box :).  So we could use a SCEV expert here.]
>>>>>>
>>>>>> In bounds_of_var_in_loop, evolution_part_in_loop_num is returning NULL:
>>>>>>
>>>>>>      step = evolution_part_in_loop_num (chrec, loop->num);
>>>
>>> (*)
>>>
>>>>>
>>>>> It means that Var doesn't vary in the loop.
>>>>> That is, chrec isn't a polynomial chrec.
>>>>
>>>> That's what I thought, but it is:
>>>>
>>>> (gdb) p chrec
>>>> $6 = <polynomial_chrec 0x7ffff0e2a820>
>>>> (gdb) dd chrec
>>>> {0, +, 1}_2
>>>>
>>>> evolution_part_in_loop_num() is returning NULL deep in
>>>> chrec_component_in_loop_num():
>>>>
>>>>         default:
>>>> =>    if (right)
>>>>            return NULL_TREE;
>>>>          else
>>>>            return chrec;
>>>>
>>>> Do you have any suggestions?
>>>
>>> I can only guess (w/o a testcase) that loop->num at (*) is not 2 and thus that
>>> chrec does not evolve in the loop we're asking.  But this doesn't make much
>>> sense with the constraints we are calling this function (a loop header PHI
>>> with loop == the loop and stmt a loop header PHI and var the PHIs lhs).
>>>
>>> OK, so looking at the testcase you're doing
>>>
>>> 492           class loop *l = loop_containing_stmt (phi);
>>> 493           if (l)
>>> 494             {
>>> 495               range_of_ssa_name_with_loop_info (loop_range,
>>> phi_def, l, phi);
>>>
>>> but 'l' isn't a loop, it's the loop tree root.  Change to
>>>
>>>     if (l && loop_outer (l))
>>
>> Woah.  I did not expect that.
>>
>> A quick peek shows that all users of bounds_of_var_in_loop (through
>> adjust_range_with_scev) predicate the call with:
>>
>>          && l->header == gimple_bb (phi))
>>
>> If this check is similar to the loop_outer(l) you suggest, could we
>> perhaps push this check (and/or the loop_outer one) into
>> bounds_of_var_in_loop itself and remove it from all the callers?  It
>> seems cleaner to have this check in one place, than in three different
>> places.  That is, unless the l->header check is altogether a different
>> thing than loop_outer(l).
> 
> The code also works for stmts other than PHIs (or stmts in other blocks
> than the loop header), so IMHO is not appropriate for bounds_of_var_in_loop.

Ahhh.   Makes perfect sense.

The patch below passes tests.  I've pushed it.

Thanks.
Aldy

gcc/ChangeLog:

	PR tree-optimization/97396
	* gimple-range.cc (gimple_ranger::range_of_phi): Do not call
	range_of_ssa_name_with_loop_info with the loop tree root.

gcc/testsuite/ChangeLog:

	* gcc.dg/pr97396.c: New test.
---
  gcc/gimple-range.cc            |  2 +-
  gcc/testsuite/gcc.dg/pr97396.c | 23 +++++++++++++++++++++++
  2 files changed, 24 insertions(+), 1 deletion(-)
  create mode 100644 gcc/testsuite/gcc.dg/pr97396.c

diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index 2ca86ed0e4c..999d631c5ee 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -490,7 +490,7 @@ gimple_ranger::range_of_phi (irange &r, gphi *phi)
      {
        value_range loop_range;
        class loop *l = loop_containing_stmt (phi);
-      if (l)
+      if (l && loop_outer (l))
          {
  	  range_of_ssa_name_with_loop_info (loop_range, phi_def, l, phi);
  	  if (!loop_range.varying_p ())
diff --git a/gcc/testsuite/gcc.dg/pr97396.c b/gcc/testsuite/gcc.dg/pr97396.c
new file mode 100644
index 00000000000..d992c11f238
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr97396.c
@@ -0,0 +1,23 @@
+// { dg-do compile }
+// { dg-options "-O1 -ftree-vrp" }
+// { dg-additional-options "-m32" { target { i?86-*-* x86_64-*-* } } }
+
+unsigned int
+po (char *os, unsigned int al)
+{
+  for (;;)
+    {
+      int qx = 0;
+
+      while (al < 1)
+        {
+          char *cw;
+
+          cw = os + qx;
+          if (cw)
+            return al + qx;
+
+          qx += sizeof *cw;
+        }
+    }
+}
-- 
2.26.2