* Re: [PATCH] Fix allocation of reg_known_value
@ 2012-11-29 22:51 Uros Bizjak
2012-11-30 10:43 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Uros Bizjak @ 2012-11-29 22:51 UTC (permalink / raw)
To: gcc-patches; +Cc: Paolo Bonzini, Steven Bosscher
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.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] Fix allocation of reg_known_value 2012-11-29 22:51 [PATCH] Fix allocation of reg_known_value Uros Bizjak @ 2012-11-30 10:43 ` Paolo Bonzini 2012-11-30 10:43 ` Steven Bosscher 0 siblings, 1 reply; 10+ messages in thread From: Paolo Bonzini @ 2012-11-30 10:43 UTC (permalink / raw) To: Uros Bizjak; +Cc: gcc-patches, Steven Bosscher 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix allocation of reg_known_value 2012-11-30 10:43 ` Paolo Bonzini @ 2012-11-30 10:43 ` Steven Bosscher 2012-11-30 12:34 ` Uros Bizjak 0 siblings, 1 reply; 10+ messages in thread From: Steven Bosscher @ 2012-11-30 10:43 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Uros Bizjak, gcc-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix allocation of reg_known_value 2012-11-30 10:43 ` Steven Bosscher @ 2012-11-30 12:34 ` Uros Bizjak 2012-11-30 13:17 ` Uros Bizjak 2012-11-30 14:54 ` Steven Bosscher 0 siblings, 2 replies; 10+ messages in thread From: Uros Bizjak @ 2012-11-30 12:34 UTC (permalink / raw) To: Steven Bosscher; +Cc: Paolo Bonzini, gcc-patches 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix allocation of reg_known_value 2012-11-30 12:34 ` Uros Bizjak @ 2012-11-30 13:17 ` Uros Bizjak 2012-11-30 14:54 ` Steven Bosscher 1 sibling, 0 replies; 10+ messages in thread From: Uros Bizjak @ 2012-11-30 13:17 UTC (permalink / raw) To: Steven Bosscher; +Cc: Paolo Bonzini, gcc-patches 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix allocation of reg_known_value 2012-11-30 12:34 ` Uros Bizjak 2012-11-30 13:17 ` Uros Bizjak @ 2012-11-30 14:54 ` Steven Bosscher 2012-11-30 15:10 ` Steven Bosscher 2012-11-30 15:57 ` Uros Bizjak 1 sibling, 2 replies; 10+ messages in thread From: Steven Bosscher @ 2012-11-30 14:54 UTC (permalink / raw) To: Uros Bizjak; +Cc: Paolo Bonzini, gcc-patches, Alexandre Oliva 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix allocation of reg_known_value 2012-11-30 14:54 ` Steven Bosscher @ 2012-11-30 15:10 ` Steven Bosscher 2012-11-30 15:57 ` Uros Bizjak 1 sibling, 0 replies; 10+ messages in thread From: Steven Bosscher @ 2012-11-30 15:10 UTC (permalink / raw) To: Uros Bizjak; +Cc: Paolo Bonzini, gcc-patches, Alexandre Oliva 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix allocation of reg_known_value 2012-11-30 14:54 ` Steven Bosscher 2012-11-30 15:10 ` Steven Bosscher @ 2012-11-30 15:57 ` Uros Bizjak 1 sibling, 0 replies; 10+ messages in thread From: Uros Bizjak @ 2012-11-30 15:57 UTC (permalink / raw) To: Steven Bosscher; +Cc: Paolo Bonzini, gcc-patches, Alexandre Oliva [-- 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)) ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] Fix allocation of reg_known_value @ 2012-11-27 16:01 Paolo Bonzini 2012-11-27 16:32 ` Steven Bosscher 0 siblings, 1 reply; 10+ messages in thread From: Paolo Bonzini @ 2012-11-27 16:01 UTC (permalink / raw) To: gcc-patches; +Cc: stevenb.gcc 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. */ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix allocation of reg_known_value 2012-11-27 16:01 Paolo Bonzini @ 2012-11-27 16:32 ` Steven Bosscher 0 siblings, 0 replies; 10+ messages in thread From: Steven Bosscher @ 2012-11-27 16:32 UTC (permalink / raw) To: Paolo Bonzini; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-11-30 15:11 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-11-29 22:51 [PATCH] Fix allocation of reg_known_value Uros Bizjak 2012-11-30 10:43 ` Paolo Bonzini 2012-11-30 10:43 ` Steven Bosscher 2012-11-30 12:34 ` Uros Bizjak 2012-11-30 13:17 ` Uros Bizjak 2012-11-30 14:54 ` Steven Bosscher 2012-11-30 15:10 ` Steven Bosscher 2012-11-30 15:57 ` Uros Bizjak -- strict thread matches above, loose matches on Subject: below -- 2012-11-27 16:01 Paolo Bonzini 2012-11-27 16:32 ` Steven Bosscher
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).