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

[Daniel Berlin <dberlin at dberlin dot org>]

It should be, Richard said:
"The patch itself is ok as soon as it no longer exposes that bug
and with added comments before the dominator checks (similar
to the existing check"


On Tue, Jun 23, 2009 at 4:40 AM, Carrot Wei<carrot@google.com> wrote:
> 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
>>>
>>
>