| Message ID | CAD57uCcQ0ZAxzgyOKOu28GoURGULe3HTKkWj1L_Go-kiY7JsKg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi Alan, On 10 August 2015 at 18:02, Alan Lawrence <alan.lawrence@arm.com> wrote: > Yvan Roux wrote: >> >> Hi, >> >> this patch is a fix for pr27127. It avoids splitting the DI registers >> into SI ones if it is not allowed, which breaks the introduced loop. >> I haven't added a testcase as the bug is already exhibited by several >> regressions (like g++.dg/ext/attribute-test-2.C or g++.dg/eh/simd-1.C) >> but I can add one if you think it is needed. Cross built and >> regtested on trunk and gcc-5 branch and the regression mentioned in >> https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00216.html is not >> observed. >> >> Is it ok for trunk and branch ? >> >> Thanks, >> Yvan >> >> gcc/ >> >> PR target/67127 >> * config/arm/arm.md (movdi): Avoid forbidden modes changed. > > > I've just looked into the above 2 testcases on armeb-none-eabi, and in both > cases the infinite loop is due to an ldrd/strd with base register 16. So not > an odd-numbered physical register, but rather something that isn't a > physical register at all. Yes in big-endian DI mode value are stored into VFP registers, and here register 16 is the first of them s0. Just in case you want to do more test, the issue can be seen with a oneline testcase: __attribute__((__vector_size__(2 * sizeof(int)))) int fn1() {} > I observe that FIRST_VIRTUAL_REGISTER is 104, whereas LAST_ARM_REG is 15. So > it might be that the pattern should check against the latter instead of the > former - as arm_hard_regno_mode_ok does... yes, when checking that that the operand register number is lower or equals to LAST_ARM_REGNUM the infinite loop is avoided. I haven't pass a full validation so far, but it has the same effect than checking that the changing mode is authorized. If you think that this checking makes more sense, I can rerun a full valid. Thanks, Yvan yes > --Alan >
On 11 August 2015 at 12:28, Ramana Radhakrishnan <ramana.radhakrishnan@foss.arm.com> wrote: >> >> Yes in big-endian DI mode value are stored into VFP registers, and >> here register 16 is the first of them s0. Just in case you want to do >> more test, the issue can be seen with a oneline testcase: >> >> __attribute__((__vector_size__(2 * sizeof(int)))) int fn1() {} > > Yep we may well have DImode values in the VFP register bank. > >> >>> I observe that FIRST_VIRTUAL_REGISTER is 104, whereas LAST_ARM_REG is 15. So >>> it might be that the pattern should check against the latter instead of the >>> former - as arm_hard_regno_mode_ok does... >> >> yes, when checking that that the operand register number is lower or >> equals to LAST_ARM_REGNUM the infinite loop is avoided. I haven't >> pass a full validation so far, but it has the same effect than >> checking that the changing mode is authorized. If you think that this >> checking makes more sense, I can rerun a full valid. > > > > It sounds to me like checking against LAST_ARM_REGNUM is the better approach. The additional test in the movdi expander was to prevent illegitimate ldrd / strd instructions from being generated. > > Thus, OK if there are no regressions - Please give it some time to bake on trunk before backporting to FSF 5 and / or do some additional validation (including the regression testsuite) before doing the backport. No regressions on trunk for both armeb-linux-gnueabihf and arm-linux-gnueabihf, thus I've committed it as r226811. I'll validate the backport to FSF 5 before committing it. Thanks Yvan > > > regards > Ramana > > >> >> Thanks, >> Yvan >> >> >> yes >> >> >>> --Alan >>>
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md index be51c77..d89e853 100644 --- a/gcc/config/arm/arm.md +++ b/gcc/config/arm/arm.md @@ -5482,7 +5482,8 @@ operands[1] = force_reg (DImode, operands[1]); } if (REG_P (operands[0]) && REGNO (operands[0]) < FIRST_VIRTUAL_REGISTER - && !HARD_REGNO_MODE_OK (REGNO (operands[0]), DImode)) + && !HARD_REGNO_MODE_OK (REGNO (operands[0]), DImode) + && !REG_CANNOT_CHANGE_MODE_P (REGNO (operands[0]), DImode, SImode)) { /* Avoid LDRD's into an odd-numbered register pair in ARM state when expanding function calls. */ @@ -5501,7 +5502,8 @@ DONE; } else if (REG_P (operands[1]) && REGNO (operands[1]) < FIRST_VIRTUAL_REGISTER - && !HARD_REGNO_MODE_OK (REGNO (operands[1]), DImode)) + && !HARD_REGNO_MODE_OK (REGNO (operands[1]), DImode) + && !REG_CANNOT_CHANGE_MODE_P (REGNO (operands[1]), DImode, SImode)) { /* Avoid STRD's from an odd-numbered register pair in ARM state when expanding function prologue. */
Hi, this patch is a fix for pr27127. It avoids splitting the DI registers into SI ones if it is not allowed, which breaks the introduced loop. I haven't added a testcase as the bug is already exhibited by several regressions (like g++.dg/ext/attribute-test-2.C or g++.dg/eh/simd-1.C) but I can add one if you think it is needed. Cross built and regtested on trunk and gcc-5 branch and the regression mentioned in https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00216.html is not observed. Is it ok for trunk and branch ? Thanks, Yvan gcc/ PR target/67127 * config/arm/arm.md (movdi): Avoid forbidden modes changed.