Re: [PATCH] Fix allocation of reg_known_value

Re: [PATCH] Fix allocation of reg_known_value

[Uros Bizjak]

Hello!

> When changing reg_known_value to a VEC, Steven sneaked in a small
> semantic change.  That change has the advantage of improving compilation
> time substantially on some testcases (including PR55489), but it is a
> bit heavy-handed: it also makes set_known_reg_value a no-op, while
> get_known_reg_value will always return NULL.
>
> This patch fixes the VEC usage.  Bootstrap/regtest in progress, but I'm
> going to commit this as obvious as soon as bootstrapping finishes.
>
> 2012-11-26  Paolo Bonzini  <pbonzini@redhat.com>
>
> 	* alias.c (init_alias_analysis): Fix allocation of reg_known_value.
>
> Index: ../../gcc/alias.c
> ===================================================================
> --- ../../gcc/alias.c	(revisione 193853)
> +++ ../../gcc/alias.c	(copia locale)
> @@ -2808,7 +2808,7 @@ init_alias_analysis (void)
>
>    timevar_push (TV_ALIAS_ANALYSIS);
>
> -  vec_alloc (reg_known_value, maxreg - FIRST_PSEUDO_REGISTER);
> +  vec_safe_grow_cleared (reg_known_value, maxreg - FIRST_PSEUDO_REGISTER);
>    reg_known_equiv_p = sbitmap_alloc (maxreg - FIRST_PSEUDO_REGISTER);
>
>    /* If we have memory allocated from the previous run, use it.  */

This one-liner causes following runtime test failure [1] for
alphaev68-linux-gnu:

FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
-fomit-frame-pointer -finline-functions
FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
-fomit-frame-pointer -finline-functions -funroll-loops
FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
-fbounds-check

The patch miscompiles libgfortran library.

I will provide more info tomorrow, any hint what/where should I look
for differences?

[1] http://gcc.gnu.org/ml/gcc-testresults/2012-11/msg02492.html

Uros.

Re: [PATCH] Fix allocation of reg_known_value

[Paolo Bonzini]

Il 29/11/2012 23:47, Uros Bizjak ha scritto:
> This one-liner causes following runtime test failure [1] for
> alphaev68-linux-gnu:
> 
> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
> -fomit-frame-pointer -finline-functions
> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
> -fomit-frame-pointer -finline-functions -funroll-loops
> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
> -fbounds-check
> 
> The patch miscompiles libgfortran library.
> 
> I will provide more info tomorrow, any hint what/where should I look
> for differences?

Anywhere.  The bug disabled a large part of alias analysis.

Perhaps you can bisect it and backport the fix along the steps.

Paolo

Re: [PATCH] Fix allocation of reg_known_value

[Steven Bosscher]

On Fri, Nov 30, 2012 at 11:39 AM, Paolo Bonzini <bonzini@gnu.org> wrote:
> Il 29/11/2012 23:47, Uros Bizjak ha scritto:
>> This one-liner causes following runtime test failure [1] for
>> alphaev68-linux-gnu:
>>
>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>> -fomit-frame-pointer -finline-functions
>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>> -fomit-frame-pointer -finline-functions -funroll-loops
>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>> -fbounds-check
>>
>> The patch miscompiles libgfortran library.
>>
>> I will provide more info tomorrow, any hint what/where should I look
>> for differences?
>
> Anywhere.  The bug disabled a large part of alias analysis.
>
> Perhaps you can bisect it and backport the fix along the steps.

Or open a PR and assign it to me, ultimately I'm responsible for this breakage.

Ciao!
Steven

Re: [PATCH] Fix allocation of reg_known_value

[Uros Bizjak]

On Fri, Nov 30, 2012 at 11:42 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Fri, Nov 30, 2012 at 11:39 AM, Paolo Bonzini <bonzini@gnu.org> wrote:
>> Il 29/11/2012 23:47, Uros Bizjak ha scritto:
>>> This one-liner causes following runtime test failure [1] for
>>> alphaev68-linux-gnu:
>>>
>>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>>> -fomit-frame-pointer -finline-functions
>>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>>> -fomit-frame-pointer -finline-functions -funroll-loops
>>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>>> -fbounds-check
>>>
>>> The patch miscompiles libgfortran library.
>>>
>>> I will provide more info tomorrow, any hint what/where should I look
>>> for differences?
>>
>> Anywhere.  The bug disabled a large part of alias analysis.
>>
>> Perhaps you can bisect it and backport the fix along the steps.
>
> Or open a PR and assign it to me, ultimately I'm responsible for this breakage.

I have found the problem. Luckily, it is the testcase that is
miscompiled. The problem is in sched1 pass that moves write to an
address "addrX" that happens to be aliased with AND mutilated address
"addrY & -7". The improved alias analysis does not notice that the
write is inside (addr & -7) region, allowing write to be moved after
read.

I'm preparing a detailed PR.

Thanks,
Uros.

Re: [PATCH] Fix allocation of reg_known_value

[Uros Bizjak]

On Fri, Nov 30, 2012 at 1:29 PM, Uros Bizjak <ubizjak@gmail.com> wrote:

>>>> This one-liner causes following runtime test failure [1] for
>>>> alphaev68-linux-gnu:
>>>>
>>>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>>>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>>>> -fomit-frame-pointer -finline-functions
>>>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>>>> -fomit-frame-pointer -finline-functions -funroll-loops
>>>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>>>> -fbounds-check
>>>>
>>>> The patch miscompiles libgfortran library.
>>>>
>>>> I will provide more info tomorrow, any hint what/where should I look
>>>> for differences?
>>>
>>> Anywhere.  The bug disabled a large part of alias analysis.
>>>
>>> Perhaps you can bisect it and backport the fix along the steps.
>>
>> Or open a PR and assign it to me, ultimately I'm responsible for this breakage.
>
> I have found the problem. Luckily, it is the testcase that is
> miscompiled. The problem is in sched1 pass that moves write to an
> address "addrX" that happens to be aliased with AND mutilated address
> "addrY & -7". The improved alias analysis does not notice that the
> write is inside (addr & -7) region, allowing write to be moved after
> read.
>
> I'm preparing a detailed PR.

Reported as PR55547.

Uros.

Re: [PATCH] Fix allocation of reg_known_value

[Steven Bosscher]

On Fri, Nov 30, 2012 at 1:29 PM, Uros Bizjak wrote:
> On Fri, Nov 30, 2012 at 11:42 AM, Steven Bosscher wrote:
>> On Fri, Nov 30, 2012 at 11:39 AM, Paolo Bonzini wrote:
>>> Il 29/11/2012 23:47, Uros Bizjak ha scritto:
>>>> This one-liner causes following runtime test failure [1] for
>>>> alphaev68-linux-gnu:
>>>>
>>>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>>>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>>>> -fomit-frame-pointer -finline-functions
>>>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>>>> -fomit-frame-pointer -finline-functions -funroll-loops
>>>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>>>> -fbounds-check
>>>>
>>>> The patch miscompiles libgfortran library.
>>>>
>>>> I will provide more info tomorrow, any hint what/where should I look
>>>> for differences?
>>>
>>> Anywhere.  The bug disabled a large part of alias analysis.
>>>
>>> Perhaps you can bisect it and backport the fix along the steps.
>>
>> Or open a PR and assign it to me, ultimately I'm responsible for this breakage.
>
> I have found the problem. Luckily, it is the testcase that is
> miscompiled. The problem is in sched1 pass that moves write to an
> address "addrX" that happens to be aliased with AND mutilated address
> "addrY & -7". The improved alias analysis does not notice that the
> write is inside (addr & -7) region, allowing write to be moved after
> read.

Based on this description of the problem, I remembered these patches
from Alexandre:

2012-07-06  Alexandre Oilva  <>

        PR rtl-optimization/53827
        PR debug/53671
        PR debug/49888
        * alias.c (memrefs_conflict_p): Adjust offset and size by the
        same amount for alignment ANDs.

2012-06-21  Alexandre Oliva  <>

        PR debug/53671
        PR debug/49888
        * alias.c (memrefs_conflict_p): Improve handling of AND for alignment.


They are r188868 and r189325. Can you try revert these and see if that
fixes your problem (probably at the expense of some other problem
returning, but at least it'd be a good place to start looking
further...).

Ciao!
Steven

Re: [PATCH] Fix allocation of reg_known_value

[Steven Bosscher]

On Fri, Nov 30, 2012 at 3:51 PM, Steven Bosscher wrote:
> They are r188868 and r189325.

This'd be the diff:
http://gcc.gnu.org/viewcvs/trunk/gcc/alias.c?r1=188448&r2=189325&pathrev=189325

Ciao!
Steven

Re: [PATCH] Fix allocation of reg_known_value

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 2158 bytes --]

On Fri, Nov 30, 2012 at 3:51 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:

>>>>> This one-liner causes following runtime test failure [1] for
>>>>> alphaev68-linux-gnu:
>>>>>
>>>>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>>>>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>>>>> -fomit-frame-pointer -finline-functions
>>>>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>>>>> -fomit-frame-pointer -finline-functions -funroll-loops
>>>>> FAIL: gfortran.fortran-torture/execute/save_1.f90 execution,  -O2
>>>>> -fbounds-check
>>>>>
>>>>> The patch miscompiles libgfortran library.
>>>>>
>>>>> I will provide more info tomorrow, any hint what/where should I look
>>>>> for differences?
>>>>
>>>> Anywhere.  The bug disabled a large part of alias analysis.
>>>>
>>>> Perhaps you can bisect it and backport the fix along the steps.
>>>
>>> Or open a PR and assign it to me, ultimately I'm responsible for this breakage.
>>
>> I have found the problem. Luckily, it is the testcase that is
>> miscompiled. The problem is in sched1 pass that moves write to an
>> address "addrX" that happens to be aliased with AND mutilated address
>> "addrY & -7". The improved alias analysis does not notice that the
>> write is inside (addr & -7) region, allowing write to be moved after
>> read.
>
> Based on this description of the problem, I remembered these patches
> from Alexandre:
>
> 2012-07-06  Alexandre Oilva  <>
>
>         PR rtl-optimization/53827
>         PR debug/53671
>         PR debug/49888
>         * alias.c (memrefs_conflict_p): Adjust offset and size by the
>         same amount for alignment ANDs.
>
> 2012-06-21  Alexandre Oliva  <>
>
>         PR debug/53671
>         PR debug/49888
>         * alias.c (memrefs_conflict_p): Improve handling of AND for alignment.
>
>
> They are r188868 and r189325. Can you try revert these and see if that
> fixes your problem (probably at the expense of some other problem
> returning, but at least it'd be a good place to start looking
> further...).

Yes backing out these revisions with attached patch fixes the runtime failure!

Uros.

[-- Attachment #2: alias.diff --]
[-- Type: application/octet-stream, Size: 1219 bytes --]

Index: alias.c
===================================================================
--- alias.c	(revision 193992)
+++ alias.c	(working copy)
@@ -2103,6 +2103,10 @@
      alignment into account.  */
   if (GET_CODE (x) == AND && CONST_INT_P (XEXP (x, 1)))
     {
+      if (GET_CODE (y) == AND || ysize < -INTVAL (XEXP (x, 1)))
+	xsize = -1;
+      return memrefs_conflict_p (xsize, canon_rtx (XEXP (x, 0)), ysize, y, c);
+#if 0
       HOST_WIDE_INT sc = INTVAL (XEXP (x, 1));
       unsigned HOST_WIDE_INT uc = sc;
       if (xsize > 0 && sc < 0 && -uc == (uc & -uc))
@@ -2112,9 +2116,14 @@
 	  return memrefs_conflict_p (xsize, canon_rtx (XEXP (x, 0)),
 				     ysize, y, c);
 	}
+#endif
     }
   if (GET_CODE (y) == AND && CONST_INT_P (XEXP (y, 1)))
     {
+      if (GET_CODE (x) == AND || xsize < -INTVAL (XEXP (y, 1)))
+	ysize = -1;
+      return memrefs_conflict_p (xsize, x, ysize, canon_rtx (XEXP (y, 0)), c);
+#if 0
       HOST_WIDE_INT sc = INTVAL (XEXP (y, 1));
       unsigned HOST_WIDE_INT uc = sc;
       if (ysize > 0 && sc < 0 && -uc == (uc & -uc))
@@ -2124,6 +2133,7 @@
 	  return memrefs_conflict_p (xsize, x,
 				     ysize, canon_rtx (XEXP (y, 0)), c);
 	}
+#endif
     }
 
   if (CONSTANT_P (x))

Re: [PATCH] Fix allocation of reg_known_value

[Steven Bosscher]

On Tue, Nov 27, 2012 at 5:00 PM, Paolo Bonzini wrote:
> When changing reg_known_value to a VEC, Steven sneaked in a small
> semantic change.

I don't sneak, I just tend to do stupid things from time to time ;-)

Thanks for catching this, and sorry for the breakage!

Ciao!
Steven

[PATCH] Fix allocation of reg_known_value

[Paolo Bonzini]

When changing reg_known_value to a VEC, Steven sneaked in a small
semantic change.  That change has the advantage of improving compilation
time substantially on some testcases (including PR55489), but it is a
bit heavy-handed: it also makes set_known_reg_value a no-op, while
get_known_reg_value will always return NULL.

This patch fixes the VEC usage.  Bootstrap/regtest in progress, but I'm
going to commit this as obvious as soon as bootstrapping finishes.

Paolo

2012-11-26  Paolo Bonzini  <pbonzini@redhat.com>

	* alias.c (init_alias_analysis): Fix allocation of reg_known_value.

Index: ../../gcc/alias.c
===================================================================
--- ../../gcc/alias.c	(revisione 193853)
+++ ../../gcc/alias.c	(copia locale)
@@ -2808,7 +2808,7 @@ init_alias_analysis (void)
 
   timevar_push (TV_ALIAS_ANALYSIS);
 
-  vec_alloc (reg_known_value, maxreg - FIRST_PSEUDO_REGISTER);
+  vec_safe_grow_cleared (reg_known_value, maxreg - FIRST_PSEUDO_REGISTER);
   reg_known_equiv_p = sbitmap_alloc (maxreg - FIRST_PSEUDO_REGISTER);
 
   /* If we have memory allocated from the previous run, use it.  */