diff mbox series

[committed] Adjust v850 rotate expander to allow more cases for V850E3V5

Message ID 6bd6ed90-2b4e-4956-8392-a8a56c577d86@gmail.com
State New
Headers show
Series [committed] Adjust v850 rotate expander to allow more cases for V850E3V5 | expand

Commit Message

Jeff Law Aug. 17, 2024, 4:33 p.m. UTC 
The recent if-conversion changes tripped a failure on the v850 port.

The core underlying issue is that while the if-conversion code tries to 
do the right thing with noce_can_force_operand to determine if it can 
force an arbitrary operand into a register, it's not really a sufficient 
check.

Essentially for arithmetic codes, it checks the operands.  If the 
operands are force-able and there's a code_to_optab mapping, then it 
returns true.

code_to_optab doesn't actually check anything other than the existence 
of  a mapping in the target.  If the target pattern has restrictions 
enforced by the condition or it's an expander that is allowed to FAIL, 
then noce_can_force_operand can return true, even though we may not be 
able to directly force the operand into a register.

This came up on the v850 when we had an operand that was a rotate by a 
constant number of bits (I don't remember the count, all that's 
important about it was the count was not 8 or 16).

The v850 port has this define_expand:

  > (define_expand "rotlsi3"
>   [(parallel [(set (match_operand:SI 0 "register_operand" "")
>                    (rotate:SI (match_operand:SI 1 "register_operand" "")
>                               (match_operand:SI 2 "const_int_operand" "")))
>               (clobber (reg:CC CC_REGNUM))])]
>   "(TARGET_V850E_UP)"
>   {
>     if (INTVAL (operands[2]) != 16)  
>       FAIL; 
>   })

So the only rotate count allowed is 16 (there's a similar HI rotate with 
a count of 8).  AFAICT the rotate patterns are allowed to FAIL.  So 
naturally the expander fails and we get a testsuite regression:

> Tests that now fail, but worked before (4 tests):
> 
> v850-sim/-mgcc-abi/-msoft-float/-mv850e3v5: gcc: gcc.c-torture/execute/20100805-1.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess errors)
> v850-sim/-mgcc-abi/-msoft-float/-mv850e3v5: gcc: gcc.c-torture/execute/20100805-1.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess errors)
> v850-sim/-msoft-float/-mv850e3v5: gcc: gcc.c-torture/execute/20100805-1.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess errors)
> v850-sim/-mv850e3v5: gcc: gcc.c-torture/execute/20100805-1.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess errors)


This patch works around the problem by allowing the rotates in 
additional cases, particularly for the V850E3V5+ variants which have a 
general rotate capability.  But let's be clear, this is just a 
workaround and I expect we're going to have to revisit the code to test 
if an operand can be forced into a register.

Pushing to the trunk.
commit abfc140579682598cd178eb9d0b0160bbfafc633
Author: Jeff Law <jlaw@ventanamicro.com>
Date:   Sat Aug 17 10:30:48 2024 -0600

    Adjust v850 rotate expander to allow more cases for V850E3V5
    
    The recent if-conversion changes tripped a failure on the v850 port.
    
    The core underlying issue is that while the if-conversion code tries to do the
    right thing with noce_can_force_operand to determine if it can force an
    arbitrary operand into a register, it's not really a sufficient check.
    
    Essentially for arithmetic codes, it checks the operands.  If the operands are
    force-able and there's a code_to_optab mapping, then it returns true.
    
    code_to_optab doesn't actually check anything other than the existence of  a
    mapping in the target.  If the target pattern has restrictions enforced by the
    condition or it's an expander that is allowed to FAIL, then
    noce_can_force_operand to be true, even though we may not be able to directly
    force the operand into a register.
    
    This came up on the v850 when we had an operand that was a rotate by a constant
    number of bits (I don't remember the count, all that's important about it was
    the count was not 8 or 16).
    
    The v850 port has this define_expand:
    
     > (define_expand "rotlsi3"
    >   [(parallel [(set (match_operand:SI 0 "register_operand" "")
    >                    (rotate:SI (match_operand:SI 1 "register_operand" "")
    >                               (match_operand:SI 2 "const_int_operand" "")))
    >               (clobber (reg:CC CC_REGNUM))])]
    >   "(TARGET_V850E_UP)"
    >   {
    >     if (INTVAL (operands[2]) != 16)
    >       FAIL;
    >   })
    So the only rotate count allowed is 16 (there's a similar HI rotate with a count of 8).  AFAICT the rotate patterns are allowed to FAIL.  So naturally the expander fails and we get a testsuite regression:
    
    > Tests that now fail, but worked before (4 tests):
    >
    > v850-sim/-mgcc-abi/-msoft-float/-mv850e3v5: gcc: gcc.c-torture/execute/20100805-1.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess errors)
    > v850-sim/-mgcc-abi/-msoft-float/-mv850e3v5: gcc: gcc.c-torture/execute/20100805-1.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess errors)
    > v850-sim/-msoft-float/-mv850e3v5: gcc: gcc.c-torture/execute/20100805-1.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess errors)
    > v850-sim/-mv850e3v5: gcc: gcc.c-torture/execute/20100805-1.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess errors)
    
    This patch works around the problem by allowing the rotates in additional
    cases, particularly for the V850E3V5+ variants which have a general rotate
    capability.  But let's be clear, this is just a workaround and I expect we're
    going to have to revisit the code to test if an operand can be forced into a
    register.
    
    gcc/
            * config/v850/v850.md (rotlsi3): Allow more cases for V850E3V5+.
diff mbox series

Patch

diff --git a/gcc/config/v850/v850.md b/gcc/config/v850/v850.md
index f810a27fa2e..83cc9972673 100644
--- a/gcc/config/v850/v850.md
+++ b/gcc/config/v850/v850.md
@@ -1352,7 +1352,9 @@  (define_expand "rotlsi3"
 	      (clobber (reg:CC CC_REGNUM))])]
   "(TARGET_V850E_UP)"
   {
-    if (INTVAL (operands[2]) != 16)
+    if (TARGET_V850E3V5_UP && e3v5_shift_operand (operands[2], SImode))
+      ;
+    else if (INTVAL (operands[2]) != 16)
       FAIL;
   })