Re: [PATCH: PR/40416] stop sinking expression if the target bb post dominates from bb

[Carrot Wei <carrot at google dot com>]

The ICE has been fixed.

The ICE occurred in function i386.c:memory_address_length line 19678

    else if (REG_P (base)    // here base is 0

According to http://www.intel.com/Assets/PDF/manual/253665.pdf page 95,

The base, index, and displacement components can be used in any combination,
and any of these components can be NULL. A scale factor may be used only when
an index also is used. Each possible combination is useful for data
structures commonly used by programmers in high-level languages and assembly
language.

We shouldn't assume there is always a valid base register here. So we must
check its existence before using it.

ChangeLog:
2009-06-18  Carrot Wei  <carrot@google.com>

        PR/40416
        * tree-ssa-sink.c: stop sinking expression if the target bb post
        dominates from bb.
        * config/i386/i386.c: check existance of base register before using it.
        * testsuite/gcc.dg/tree-ssa/ssa-sink-5.c: new test case.


Test:
x86 bootstrap.
Gcc regression test for x86. No new failure found.

thanks
Carrot


Index: tree-ssa-sink.c
===================================================================
--- tree-ssa-sink.c     (revision 148565)
+++ tree-ssa-sink.c     (working copy)
@@ -384,6 +384,11 @@ statement_sink_location (gimple stmt, ba
          || sinkbb->loop_father != frombb->loop_father)
        return false;

+      /* Move the expression to a post dominator can't reduce the number of
+         executions.  */
+      if (dominated_by_p (CDI_POST_DOMINATORS, frombb, sinkbb))
+        return false;
+
       *togsi = gsi_for_stmt (use);
       return true;
     }
@@ -411,6 +416,11 @@ statement_sink_location (gimple stmt, ba
       || sinkbb->loop_father != frombb->loop_father)
     return false;

+  /* Move the expression to a post dominator can't reduce the number of
+     executions.  */
+  if (dominated_by_p (CDI_POST_DOMINATORS, frombb, sinkbb))
+    return false;
+
   *togsi = gsi_after_labels (sinkbb);

   return true;


Index: i386.c
===================================================================
--- i386.c      (revision 148565)
+++ i386.c      (working copy)
@@ -19675,7 +19675,7 @@ memory_address_length (rtx addr)
            len = 4;
        }
       /* ebp always wants a displacement.  Similarly r13.  */
-      else if (REG_P (base)
+      else if (base && REG_P (base)
               && (REGNO (base) == BP_REG || REGNO (base) == R13_REG))
        len = 1;

@@ -19684,7 +19684,7 @@ memory_address_length (rtx addr)
          /* ...like esp (or r12), which always wants an index.  */
          || base == arg_pointer_rtx
          || base == frame_pointer_rtx
-         || (REG_P (base)
+         || (base && REG_P (base)
              && (REGNO (base) == SP_REG || REGNO (base) == R12_REG)))
        len += 1;
     }


Index: ssa-sink-5.c
===================================================================
--- ssa-sink-5.c        (revision 0)
+++ ssa-sink-5.c        (revision 0)
@@ -0,0 +1,48 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -Os -fdump-tree-sink-stats" } */
+
+typedef short int16_t;
+typedef unsigned char uint8_t;
+
+void foo(int16_t runs[], uint8_t alpha[], int x, int count)
+{
+    int16_t* next_runs = runs + x;
+    uint8_t*  next_alpha = alpha + x;
+
+    while (x > 0)
+    {
+        int n = runs[0];
+
+        if (x < n)
+        {
+            alpha[x] = alpha[0];
+            runs[0] = (int16_t)(x);
+            runs[x] = (int16_t)(n - x);
+            break;
+        }
+        runs += n;
+        alpha += n;
+        x -= n;
+    }
+
+    runs = next_runs;
+    alpha = next_alpha;
+    x = count;
+
+   for (;;)
+    {
+        int n = runs[0];
+
+        if (x < n)
+        {
+            alpha[x] = alpha[0];
+            break;
+        }
+        x -= n;
+        runs += n;
+   }
+}
+
+/* We should not sink the next_runs = runs + x calculation after the loop.  */
+/* { dg-final { scan-tree-dump-times "Sunk statements:" 0 "sink" } } */
+/* { dg-final { cleanup-tree-dump "sink" } } */


On Tue, Jun 16, 2009 at 9:13 PM, Daniel Berlin<dannyb@google.com> wrote:
> On Tue, Jun 16, 2009 at 4:58 AM, Carrot Wei<carrot@google.com> wrote:
>> Hi
>>
>> This patch is to fix the bug
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40416
>>
>> If the target bb post dominates from bb and we move the expression to target
>> bb, it can't be executed less frequently. So we can't get any benefit from it.
>>
>> ChangeLog:
>> 2009-06-16 ?Carrot Wei ?<carrot@google.com>
>>
>> ? ? ? ?PR/40416
>> ? ? ? ?* tree-ssa-sink.c: add code to stop sinking expression if the target
>> ? ? ? ?bb post dominates from bb.
>>
>> Test:
>> x86 bootstrap.
>> Gcc regression tests for x86.
>> One new ICE testsuite/gcc.dg/pr35065.c. It is occurred in a very late rtl pass
>> machine_reorg with an invalid x86 address expression. It should be caused by
>> an unknown bug in rtl passes.
>>
>
> Two quick things:
> 1. You have to fix the bug it exposes, sadly.
> You are welcome to ask for help if you can't figure it out and need assistance.
> 2. Can you please add the small testcase you had as ssa-sink-5.c?
> If you aren't familiar with how the testsuite works, you should just
> be able to copy the lines that appear in
> testsuite/gcc.dg/tree-ssa/ssa-sink-1.c and change it from Sunk
> Statements: 1 to Sunk Statements: 0.
>
>
> Otherwise, looks good!
>