[PATCH] PR target/67127: [ARM] Avoiding odd-number ldrd/strd in movdi introduced a regression on armeb-linux-gnueabihf

[PATCH] PR target/67127: [ARM] Avoiding odd-number ldrd/strd in movdi introduced a regression on armeb-linux-gnueabihf

[Yvan Roux]

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

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.

[-- Attachment #2: pr67127.diff --]
[-- Type: text/plain, Size: 1048 bytes --]

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.  */

Re: [PATCH] PR target/67127: [ARM] Avoiding odd-number ldrd/strd in movdi introduced a regression on armeb-linux-gnueabihf

[Alan Lawrence]

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.

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...

--Alan

Re: [PATCH] PR target/67127: [ARM] Avoiding odd-number ldrd/strd in movdi introduced a regression on armeb-linux-gnueabihf

[Yvan Roux]

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
>

Re: [PATCH] PR target/67127: [ARM] Avoiding odd-number ldrd/strd in movdi introduced a regression on armeb-linux-gnueabihf

[Ramana Radhakrishnan]

> 
> 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.



regards
Ramana


> 
> Thanks,
> Yvan
> 
> 
> yes
> 
> 
>> --Alan
>>

Re: [PATCH] PR target/67127: [ARM] Avoiding odd-number ldrd/strd in movdi introduced a regression on armeb-linux-gnueabihf

[Yvan Roux]

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
>>>