Re: [PATCH v3] RISC-V: elide unnecessary sign extend when expanding cmp_and_jump

[Patrick O'Neill]

On 10/29/23 20:21, 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.
Testing on the pre-commit CI has completed.
https://github.com/ewlu/gcc-precommit-ci/issues/499#issuecomment-1784446631

The patch was applied to this baseline:
https://github.com/gcc-mirror/gcc/commit/c6929b085580cf00cbc52b0f5b0afe2b9caa2a22

and no new failures or resolved failures were found when running the 
testsuite.

Tested-by: Patrick O'Neill <patr...@rivosinc.com>

Thanks!
Patrick

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 <vine...@rivosinc.com>
---
Changes since v2:
   - Fix linting issues flagged by pre-commit CI
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 ca9a2ca81d53..269beb3b159b 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) 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);
        }
      }
  }