This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Ready to install?

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.

This fix triggered an ICE 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-23  Carrot Wei  <carrot@google.com>

        PR/40416
        * tree-ssa-sink.c (statement_sink_location): Stop sinking expression
        if the target bb post dominates from bb.
        * config/i386/i386.c (memory_address_length): Check existence of base
        register before using it.


2009-06-23  Carrot Wei  <carrot@google.com>

        PR/40416
        * gcc.dg/tree-ssa/ssa-sink-5.c: New testcase.


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 Thu, Jun 18, 2009 at 4:49 PM, Carrot Wei<carrot@google.com> wrote:
> Thank you, Paolo.
>
> On Thu, Jun 18, 2009 at 3:41 PM, Paolo Bonzini<paolo.bonzini@gmail.com> wrote:
>>
>>> 2009-06-18 ?Carrot Wei ?<carrot@google.com>
>>>
>>> ? ? ? ?PR/40416
>>> ? ? ? ?* tree-ssa-sink.c (statement_sink_location): Stop sinking
>>> expression
>>> ? ? ? ?if the target bb post dominates from bb.
>>> ? ? ? ?* config/i386/i386.c (memory_address_length): Check existance of
>>> base
>>> ? ? ? ?register before using it.
>>> ? ? ? ?* gcc.dg/tree-ssa/ssa-sink-5.c: New testcase.
>>
>> The correct format is
>>
>> 2009-06-18 ?Carrot Wei ?<carrot@google.com>
>>
>> ? ? ? ?PR/40416
>> ? ? ? ?* tree-ssa-sink.c (statement_sink_location): Stop sinking
>> ? ? ? ?expression if the target bb post dominates from bb.
>> ? ? ? ?* config/i386/i386.c (memory_address_length): Check existance of
>> ? ? ? ?base register before using it.
>>
>> 2009-06-18 ?Carrot Wei ?<carrot@google.com>
>>
>> ? ? ? ?PR/40416
>> ? ? ? ?* gcc.dg/tree-ssa/ssa-sink-5.c: New testcase.
>>
>> The double header means that you will put that in a separate ChangeLog file.
>> ?You can optionally put something like "testsuite:" before it to say which
>> ChangeLog file will host the second entry.
>>
>> Paolo
>>
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]