diff mbox series

[v2] RISC-V: elide unnecessary sign extend when expanding cmp_and_jump

Message ID 20231030020407.754075-1-vineetg@rivosinc.com
State Accepted
Headers
Series [v2] RISC-V: elide unnecessary sign extend when expanding cmp_and_jump |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Vineet Gupta Oct. 30, 2023, 2:04 a.m. UTC 
  RV64 compare and branch instructions only support 64-bit operands.
At Expand time, the backend conservatively zero/sign extends
its operands even if not needed, such as incoming 32-bit function args
which ABI/ISA guarantee to be sign-extended already.

And subsequently REE fails to eliminate them as
   "missing defintion(s)" or "multiple definition(s)
since function args don't have explicit definition.

So during expand riscv_extend_comparands (), if an operand is a
subreg-promoted SI with inner DI, which is representative of a function
arg, just peel away the subreg to expose the DI, eliding the sign
extension. As Jeff noted this routine is also used in if-conversion so
also helps there.

Note there's currently patches floating around to improve REE and also a
new pass to eliminate unneccesary extensions, but it is still beneficial
to not generate those extra extensions in first place. It is obviously
less work for post-reload passes such as REE, but even for earlier
passes, such as combine, having to deal with one less thing and ensuing
fewer combinations is a win too.

Way too many existing tests used to observe this issue.
e.g. gcc.c-torture/compile/20190827-1.c -O2 -march=rv64gc
It elimiates the SEXT.W

Tested with rv64gc with no regressions, I'm relying on PAtrick's
pre-commit CI to do the full testing.

gcc/ChangeLog:
	* config/riscv/riscv.cc (riscv_sign_extend_if_not_subreg_prom): New.
	* (riscv_extend_comparands): Call New function on operands.

Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
Changes since v1:
  - Elide sign extension for 32-bit operarnds only
  - Apply elison for both arguments
---
 gcc/config/riscv/riscv.cc | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

Comments

Vineet Gupta Oct. 30, 2023, 3:17 a.m. UTC | #1 
On 10/29/23 19:04, Vineet Gupta wrote:
> RV64 compare and branch instructions only support 64-bit operands.
> At Expand time, the backend conservatively zero/sign extends
> its operands even if not needed, such as incoming 32-bit function args
> which ABI/ISA guarantee to be sign-extended already.
>
> And subsequently REE fails to eliminate them as
>     "missing defintion(s)" or "multiple definition(s)
> since function args don't have explicit definition.
>
> So during expand riscv_extend_comparands (), if an operand is a
> subreg-promoted SI with inner DI, which is representative of a function
> arg, just peel away the subreg to expose the DI, eliding the sign
> extension. As Jeff noted this routine is also used in if-conversion so
> also helps there.
>
> Note there's currently patches floating around to improve REE and also a
> new pass to eliminate unneccesary extensions, but it is still beneficial
> to not generate those extra extensions in first place. It is obviously
> less work for post-reload passes such as REE, but even for earlier
> passes, such as combine, having to deal with one less thing and ensuing
> fewer combinations is a win too.
>
> Way too many existing tests used to observe this issue.
> e.g. gcc.c-torture/compile/20190827-1.c -O2 -march=rv64gc
> It elimiates the SEXT.W
>
> Tested with rv64gc with no regressions, I'm relying on PAtrick's
> pre-commit CI to do the full testing.
>
> gcc/ChangeLog:
> 	* config/riscv/riscv.cc (riscv_sign_extend_if_not_subreg_prom): New.
> 	* (riscv_extend_comparands): Call New function on operands.
>
> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
> ---
> Changes since v1:
>    - Elide sign extension for 32-bit operarnds only
>    - Apply elison for both arguments
> ---
>   gcc/config/riscv/riscv.cc | 23 +++++++++++++++++++++--
>   1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index f2dcb0db6fbd..3af834f92977 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -3678,6 +3678,24 @@ riscv_zero_if_equal (rtx cmp0, rtx cmp1)
>   		       cmp0, cmp1, 0, 0, OPTAB_DIRECT);
>   }
>   
> +/* Helper function for riscv_extend_comparands to Sign-extend the OP.
> +   However if the OP is SI subreg promoted with an inner DI, such as
> +       (subreg/s/v:SI (reg/v:DI 150 [ xx ]) 0)
> +   just peel off the SUBREG to get DI, avoiding extraneous extension.  */
> +
> +static void
> +riscv_sign_extend_if_not_subreg_prom (rtx *op)
> +{
> +  if (GET_MODE(*op) == SImode

Weird, this is flagged in pre-commit CI, but contrib scripts think this 
is ok

contrib/gcc-changelog/git_check_commit.py
Checking 3d9823e2fb1c1f99bb875bffd999ab8dafd53a50: OK


> +      && GET_CODE (*op) == SUBREG
> +      && SUBREG_PROMOTED_VAR_P (*op)
> +      && GET_MODE_SIZE (GET_MODE (XEXP (*op, 0))).to_constant ()
> +	 == GET_MODE_SIZE (word_mode))
> +    *op = XEXP (*op, 0);
> +  else
> +    *op = gen_rtx_SIGN_EXTEND (word_mode, *op);
> +}
> +
>   /* Sign- or zero-extend OP0 and OP1 for integer comparisons.  */
>   
>   static void
> @@ -3707,9 +3725,10 @@ riscv_extend_comparands (rtx_code code, rtx *op0, rtx *op1)
>   	}
>         else
>   	{
> -	  *op0 = gen_rtx_SIGN_EXTEND (word_mode, *op0);
> +	  riscv_sign_extend_if_not_subreg_prom(op0);
> +
>   	  if (*op1 != const0_rtx)
> -	    *op1 = gen_rtx_SIGN_EXTEND (word_mode, *op1);
> +	    riscv_sign_extend_if_not_subreg_prom(op1);
>   	}
>       }
>   }
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index f2dcb0db6fbd..3af834f92977 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -3678,6 +3678,24 @@  riscv_zero_if_equal (rtx cmp0, rtx cmp1)
 		       cmp0, cmp1, 0, 0, OPTAB_DIRECT);
 }
 
+/* Helper function for riscv_extend_comparands to Sign-extend the OP.
+   However if the OP is SI subreg promoted with an inner DI, such as
+       (subreg/s/v:SI (reg/v:DI 150 [ xx ]) 0)
+   just peel off the SUBREG to get DI, avoiding extraneous extension.  */
+
+static void
+riscv_sign_extend_if_not_subreg_prom (rtx *op)
+{
+  if (GET_MODE(*op) == SImode
+      && GET_CODE (*op) == SUBREG
+      && SUBREG_PROMOTED_VAR_P (*op)
+      && GET_MODE_SIZE (GET_MODE (XEXP (*op, 0))).to_constant ()
+	 == GET_MODE_SIZE (word_mode))
+    *op = XEXP (*op, 0);
+  else
+    *op = gen_rtx_SIGN_EXTEND (word_mode, *op);
+}
+
 /* Sign- or zero-extend OP0 and OP1 for integer comparisons.  */
 
 static void
@@ -3707,9 +3725,10 @@  riscv_extend_comparands (rtx_code code, rtx *op0, rtx *op1)
 	}
       else
 	{
-	  *op0 = gen_rtx_SIGN_EXTEND (word_mode, *op0);
+	  riscv_sign_extend_if_not_subreg_prom(op0);
+
 	  if (*op1 != const0_rtx)
-	    *op1 = gen_rtx_SIGN_EXTEND (word_mode, *op1);
+	    riscv_sign_extend_if_not_subreg_prom(op1);
 	}
     }
 }