| Message ID | CAD57uCfHZuBqWbMiqGT+qDXQj37NCa9wKcEN+RpcUS3xpurYhQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
HI Kyrill, On 18 March 2015 at 11:24, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: > Hi Yvan, > > > On 18/03/15 10:19, Yvan Roux wrote: >> >> Hi, >> >> This is a fix for PR64208 where LRA loops when dealing with >> iwmmxt_arm_movdi insn. As explain in the PR, the issue was introduced >> on trunk and 4.9 branch by fix of PR rtl-optimization/60969 and then >> workaround by r211798 (-fuse-caller-save enable for ARM). >> >> The changes in IRA cost made by PR60969, changed the register class of >> this insn output from GENERAL_REGS to IWMMXT_REGS, and the >> redundancies in the insn pattern alternatives description force LRA to >> reload the pseudo, which generates the same iwmmxt_arm_movdi insn, >> which can't be resolved, and so on ... >> >> Removing the redundancies fixes the issue, as LRA find that >> alternative 8 (Uy => y) matches. >> >> This issue is present in 4.9 branch, but latent on trunk (the >> clobbering of IP and CC information added during -fuse-caller-save >> patch changed the register allocation). >> >> Cross compiled and regression tested on ARM targets (but not on an >> IWMMXT one), is it ok for trunk and 4.9 branch ? >> >> Rq: I think that adding IP and CC clobbers to >> CALL_INSN_FUNCTION_USAGE, as specified by AAPCS, in 4.9 branch is >> something we need too, I've a patch for that if you agree on that. >> >> Thanks, >> Yvan >> >> 2105-03-17 Yvan Roux <yvan.roux@linaro.org> >> >> PR target/64208 >> * config/arm/iwmmxt.md ("*iwmmxt_arm_movdi"): Cleanup redundant >> alternatives. > > > As a general note, I think this patch should include the testcase from the > PR. > I can see that it's fairly self-contained. Yes, I wanted to find one that exhibits the issue on trunk as the PR testcase doesn't, but don't find one so far. If it's fine to include a testcase that don't fail without that patch on trunk, I can include it. Cheers, Yvan
Hi, On 23 March 2015 at 17:08, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: > On Wed, Mar 18, 2015 at 10:19 AM, Yvan Roux <yvan.roux@linaro.org> wrote: >> Hi, >> >> This is a fix for PR64208 where LRA loops when dealing with >> iwmmxt_arm_movdi insn. As explain in the PR, the issue was introduced >> on trunk and 4.9 branch by fix of PR rtl-optimization/60969 and then >> workaround by r211798 (-fuse-caller-save enable for ARM). >> >> The changes in IRA cost made by PR60969, changed the register class of >> this insn output from GENERAL_REGS to IWMMXT_REGS, and the >> redundancies in the insn pattern alternatives description force LRA to >> reload the pseudo, which generates the same iwmmxt_arm_movdi insn, >> which can't be resolved, and so on ... >> >> Removing the redundancies fixes the issue, as LRA find that >> alternative 8 (Uy => y) matches. >> >> This issue is present in 4.9 branch, but latent on trunk (the >> clobbering of IP and CC information added during -fuse-caller-save >> patch changed the register allocation). >> >> Cross compiled and regression tested on ARM targets (but not on an >> IWMMXT one), is it ok for trunk and 4.9 branch ? > > > This looks sane. It doesn't look reasonable for alternatives to be > duplicating each other. > > Given I have neither the time nor the hardware to test this patch on, > I'd rather someone with an interest in iwMMX possibly folks from > Marvell can pick up testing for this patch. Ok, Thanks Ramana, I'll wait for somebody able to test it. Notice, that without this patch on the 4.9 branch, building a cross compiler which default to iWMMXT architectures ICE on that during LRA while building of libgcc. Cheers, Yvan > regards > Ramana > >> >> Rq: I think that adding IP and CC clobbers to >> CALL_INSN_FUNCTION_USAGE, as specified by AAPCS, in 4.9 branch is >> something we need too, I've a patch for that if you agree on that. >> >> Thanks, >> Yvan >> >> 2105-03-17 Yvan Roux <yvan.roux@linaro.org> >> >> PR target/64208 >> * config/arm/iwmmxt.md ("*iwmmxt_arm_movdi"): Cleanup redundant >> alternatives.
Hi Xingxing, do you know if it is possible to test this patch inside Marvell (as it is a fix for iWMMXT arch.) ? Thanks a lot Yvan On 23 March 2015 at 18:47, Yvan Roux <yvan.roux@linaro.org> wrote: > Hi, > > On 23 March 2015 at 17:08, Ramana Radhakrishnan > <ramana.gcc@googlemail.com> wrote: >> On Wed, Mar 18, 2015 at 10:19 AM, Yvan Roux <yvan.roux@linaro.org> wrote: >>> Hi, >>> >>> This is a fix for PR64208 where LRA loops when dealing with >>> iwmmxt_arm_movdi insn. As explain in the PR, the issue was introduced >>> on trunk and 4.9 branch by fix of PR rtl-optimization/60969 and then >>> workaround by r211798 (-fuse-caller-save enable for ARM). >>> >>> The changes in IRA cost made by PR60969, changed the register class of >>> this insn output from GENERAL_REGS to IWMMXT_REGS, and the >>> redundancies in the insn pattern alternatives description force LRA to >>> reload the pseudo, which generates the same iwmmxt_arm_movdi insn, >>> which can't be resolved, and so on ... >>> >>> Removing the redundancies fixes the issue, as LRA find that >>> alternative 8 (Uy => y) matches. >>> >>> This issue is present in 4.9 branch, but latent on trunk (the >>> clobbering of IP and CC information added during -fuse-caller-save >>> patch changed the register allocation). >>> >>> Cross compiled and regression tested on ARM targets (but not on an >>> IWMMXT one), is it ok for trunk and 4.9 branch ? >> >> >> This looks sane. It doesn't look reasonable for alternatives to be >> duplicating each other. >> >> Given I have neither the time nor the hardware to test this patch on, >> I'd rather someone with an interest in iwMMX possibly folks from >> Marvell can pick up testing for this patch. > > Ok, Thanks Ramana, I'll wait for somebody able to test it. Notice, > that without this patch on the 4.9 branch, building a cross compiler > which default to iWMMXT architectures ICE on that during LRA while > building of libgcc. > > Cheers, > Yvan > >> regards >> Ramana >> >>> >>> Rq: I think that adding IP and CC clobbers to >>> CALL_INSN_FUNCTION_USAGE, as specified by AAPCS, in 4.9 branch is >>> something we need too, I've a patch for that if you agree on that. >>> >>> Thanks, >>> Yvan >>> >>> 2105-03-17 Yvan Roux <yvan.roux@linaro.org> >>> >>> PR target/64208 >>> * config/arm/iwmmxt.md ("*iwmmxt_arm_movdi"): Cleanup redundant >>> alternatives.
Hi, On 23 March 2015 at 18:47, Yvan Roux <yvan.roux@linaro.org> wrote: > Hi, > > On 23 March 2015 at 17:08, Ramana Radhakrishnan > <ramana.gcc@googlemail.com> wrote: >> On Wed, Mar 18, 2015 at 10:19 AM, Yvan Roux <yvan.roux@linaro.org> wrote: >>> Hi, >>> >>> This is a fix for PR64208 where LRA loops when dealing with >>> iwmmxt_arm_movdi insn. As explain in the PR, the issue was introduced >>> on trunk and 4.9 branch by fix of PR rtl-optimization/60969 and then >>> workaround by r211798 (-fuse-caller-save enable for ARM). >>> >>> The changes in IRA cost made by PR60969, changed the register class of >>> this insn output from GENERAL_REGS to IWMMXT_REGS, and the >>> redundancies in the insn pattern alternatives description force LRA to >>> reload the pseudo, which generates the same iwmmxt_arm_movdi insn, >>> which can't be resolved, and so on ... >>> >>> Removing the redundancies fixes the issue, as LRA find that >>> alternative 8 (Uy => y) matches. >>> >>> This issue is present in 4.9 branch, but latent on trunk (the >>> clobbering of IP and CC information added during -fuse-caller-save >>> patch changed the register allocation). >>> >>> Cross compiled and regression tested on ARM targets (but not on an >>> IWMMXT one), is it ok for trunk and 4.9 branch ? >> >> >> This looks sane. It doesn't look reasonable for alternatives to be >> duplicating each other. >> >> Given I have neither the time nor the hardware to test this patch on, >> I'd rather someone with an interest in iwMMX possibly folks from >> Marvell can pick up testing for this patch. > > Ok, Thanks Ramana, I'll wait for somebody able to test it. Notice, > that without this patch on the 4.9 branch, building a cross compiler > which default to iWMMXT architectures ICE on that during LRA while > building of libgcc. I got an access to a cubox with an armada 510 and finally managed to validate this patch (~ 1week for bootstrap + make check !). So, bootstrap is ok and no regession. is it Ok for trunk and branches (the issue was observed on 4.9) ? Notice that I've only tested it for trunk and I don't plan to validate it on the branches ! ;) Thanks Yvan > Cheers, > Yvan > >> regards >> Ramana >> >>> >>> Rq: I think that adding IP and CC clobbers to >>> CALL_INSN_FUNCTION_USAGE, as specified by AAPCS, in 4.9 branch is >>> something we need too, I've a patch for that if you agree on that. >>> >>> Thanks, >>> Yvan >>> >>> 2105-03-17 Yvan Roux <yvan.roux@linaro.org> >>> >>> PR target/64208 >>> * config/arm/iwmmxt.md ("*iwmmxt_arm_movdi"): Cleanup redundant >>> alternatives.
On 6 May 2015 at 11:50, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: > On Thu, Apr 30, 2015 at 6:49 PM, Yvan Roux <yvan.roux@linaro.org> wrote: >> Hi, >> >> On 23 March 2015 at 18:47, Yvan Roux <yvan.roux@linaro.org> wrote: >>> Hi, >>> >>> On 23 March 2015 at 17:08, Ramana Radhakrishnan >>> <ramana.gcc@googlemail.com> wrote: >>>> On Wed, Mar 18, 2015 at 10:19 AM, Yvan Roux <yvan.roux@linaro.org> wrote: >>>>> Hi, >>>>> >>>>> This is a fix for PR64208 where LRA loops when dealing with >>>>> iwmmxt_arm_movdi insn. As explain in the PR, the issue was introduced >>>>> on trunk and 4.9 branch by fix of PR rtl-optimization/60969 and then >>>>> workaround by r211798 (-fuse-caller-save enable for ARM). >>>>> >>>>> The changes in IRA cost made by PR60969, changed the register class of >>>>> this insn output from GENERAL_REGS to IWMMXT_REGS, and the >>>>> redundancies in the insn pattern alternatives description force LRA to >>>>> reload the pseudo, which generates the same iwmmxt_arm_movdi insn, >>>>> which can't be resolved, and so on ... >>>>> >>>>> Removing the redundancies fixes the issue, as LRA find that >>>>> alternative 8 (Uy => y) matches. >>>>> >>>>> This issue is present in 4.9 branch, but latent on trunk (the >>>>> clobbering of IP and CC information added during -fuse-caller-save >>>>> patch changed the register allocation). >>>>> >>>>> Cross compiled and regression tested on ARM targets (but not on an >>>>> IWMMXT one), is it ok for trunk and 4.9 branch ? >>>> >>>> >>>> This looks sane. It doesn't look reasonable for alternatives to be >>>> duplicating each other. >>>> >>>> Given I have neither the time nor the hardware to test this patch on, >>>> I'd rather someone with an interest in iwMMX possibly folks from >>>> Marvell can pick up testing for this patch. >>> >>> Ok, Thanks Ramana, I'll wait for somebody able to test it. Notice, >>> that without this patch on the 4.9 branch, building a cross compiler >>> which default to iWMMXT architectures ICE on that during LRA while >>> building of libgcc. >> >> I got an access to a cubox with an armada 510 and finally managed to >> validate this patch (~ 1week for bootstrap + make check !). So, >> bootstrap is ok and no regession. is it Ok for trunk and branches >> (the issue was observed on 4.9) ? Notice that I've only tested it for >> trunk and I don't plan to validate it on the branches ! ;) > > OK for trunk - Thanks for taking the extra effort to get an armada > board to validate this on. > > it's ok for the branches only if you validate it on the branches. If > someone is interested in the bug fix they can always pick it up Ok fair enough. Thanks Ramana. Cheers, Yvan
diff --git a/gcc/config/arm/iwmmxt.md b/gcc/config/arm/iwmmxt.md index fda3c2c..d1a60ff 100644 --- a/gcc/config/arm/iwmmxt.md +++ b/gcc/config/arm/iwmmxt.md @@ -107,8 +107,8 @@ ) (define_insn "*iwmmxt_arm_movdi" - [(set (match_operand:DI 0 "nonimmediate_di_operand" "=r, r, r, r, m,y,y,yr,y,yrUy,*w, r,*w,*w, *Uv") - (match_operand:DI 1 "di_operand" "rDa,Db,Dc,mi,r,y,yr,y,yrUy,y, r,*w,*w,*Uvi,*w"))] + [(set (match_operand:DI 0 "nonimmediate_di_operand" "=r, r, r, r, m,y,y,r, y,Uy,*w, r,*w,*w, *Uv") + (match_operand:DI 1 "di_operand" "rDa,Db,Dc,mi,r,y,r,y,Uy,y, r,*w,*w,*Uvi,*w"))] "TARGET_REALLY_IWMMXT && ( register_operand (operands[0], DImode) || register_operand (operands[1], DImode))"
Hi, This is a fix for PR64208 where LRA loops when dealing with iwmmxt_arm_movdi insn. As explain in the PR, the issue was introduced on trunk and 4.9 branch by fix of PR rtl-optimization/60969 and then workaround by r211798 (-fuse-caller-save enable for ARM). The changes in IRA cost made by PR60969, changed the register class of this insn output from GENERAL_REGS to IWMMXT_REGS, and the redundancies in the insn pattern alternatives description force LRA to reload the pseudo, which generates the same iwmmxt_arm_movdi insn, which can't be resolved, and so on ... Removing the redundancies fixes the issue, as LRA find that alternative 8 (Uy => y) matches. This issue is present in 4.9 branch, but latent on trunk (the clobbering of IP and CC information added during -fuse-caller-save patch changed the register allocation). Cross compiled and regression tested on ARM targets (but not on an IWMMXT one), is it ok for trunk and 4.9 branch ? Rq: I think that adding IP and CC clobbers to CALL_INSN_FUNCTION_USAGE, as specified by AAPCS, in 4.9 branch is something we need too, I've a patch for that if you agree on that. Thanks, Yvan 2105-03-17 Yvan Roux <yvan.roux@linaro.org> PR target/64208 * config/arm/iwmmxt.md ("*iwmmxt_arm_movdi"): Cleanup redundant alternatives.