On 10/29/23 21: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.
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);
So for the wrapped test GET_MODE_SIZE stuff), add parenthesis and indent
the "==" clause. ie
&& (GET_MODE_SIZE (GET_MODE (XEXP (*op), 0))).to_constant ()
== GET_MODE_SIZE (word_mode))
Don't you also need to verify that the subreg was sign extended? The
PROMOTED_VAR_P just notes that it was promoted, not *how* it was
promoted. I think you just need to add a test like this:
&& SUBREG_PROMOTED_SIGNED_P (*op)
I don't guess you have data on how this impacts dynamic instruction
counts on anything significant do you?
OK with the formatting nit fixed and adding the additional check to
ensure the value was sign extended.
jeff
