On 10/24/23 22:01, Vineet Gupta wrote:
RV64 comapre and branch instructions only support 64-bit operands.
The backend unconditionally generates zero/sign extend at Expand time
for compare and branch operands even if they are already as such
e.g. function args which ABI guarantees to be sign-extended (at callsite).
And subsequently REE fails to eliminate them as
"missing defintion(s)"
specially for function args since they show up as missing explicit
definition.
So elide the sign extensions at Expand time for a subreg promoted var
with inner word sized value whcih doesn't need explicit sign extending
(fairly good represntative of an incoming function arg).
There are patches floating to enhance REE and/or new passes to elimiate
extensions, but it is always better to not generate them if possible,
given Expand is so early, the elimiated extend would help improve outcomes
of later passes such as combine if they have fewer operations/combinations
to deal with.
The test used was existing gcc.c-torture/execute/20020506-1.c -O2 zbb
It elimiates the SEXT.W and an additional branch
before after
------- ------
test2: test2:
sext.b a0,a0
blt a0,zero,.L15
bne a1,zero,.L17 bne a1,zero,.L17
This is marked RFC as I only ran a spot check with a directed test and
want to use Patrick's pre-commit CI to do the A/B testing for me.
gcc/ChangeLog:
* config/riscv/riscv.cc (riscv_extend_comparands): Don't
sign-extend operand if subreg promoted with inner word size.
Signed-off-by: Vineet Gupta <vine...@rivosinc.com>
---
gcc/config/riscv/riscv.cc | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index f2dcb0db6fbd..a8d12717e43d 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -3707,7 +3707,16 @@ riscv_extend_comparands (rtx_code code, rtx *op0, rtx
*op1)
}
else
{
- *op0 = gen_rtx_SIGN_EXTEND (word_mode, *op0);
+ /* A subreg promoted SI of DI could be peeled to expose DI, eliding
+ an unnecessary sign extension. */
+ if (GET_CODE (*op0) == SUBREG
+ && SUBREG_PROMOTED_VAR_P (*op0)
+ && GET_MODE_SIZE (GET_MODE (XEXP (*op0, 0))).to_constant ()
+ == GET_MODE_SIZE (word_mode))
+ *op0 = XEXP (*op0, 0);
+ else
+ *op0 = gen_rtx_SIGN_EXTEND (word_mode, *op0);
+
if (*op1 != const0_rtx)
*op1 = gen_rtx_SIGN_EXTEND (word_mode, *op1);
}
Just a quick update that testing revealed additional failures and
unpacking which took me a while and in hindsight embarrassing. I was
misunderstanding what ABI guarantees and what ISA actually does :-)
The ABI guarantees sign extension this for 32-bit things in 64-bit reg
(so in rv64, int), but *not* 8-bit things in a reg. i.e. not for char
arguments.
e.g.
uint8_t abs8(uint8_t x)
{
if (x & 0x80) // SEXT.b a4, a0
...
}
main()
{
if (abs8(128) != 127) // LI a0, 128 => ADDI a0, x0, 128
__builtin_abort();
}
So my patch was optimizing away the SEXT.B (despite claiming that subreg
prom of SI....) which is wrong.
Sure ADDI in main () sign-extends, but it does that for 12-bit imm
0x080, which comes out to 0x80, but sign-extending for char scope 0x80
would yield 0xFFFF....0x80. Hence the issue.
I'll spin a v2 after more testing.
This is slightly disappointing as it reduces the scope of optimization,
but correctness in this trade is non-negotiable :-)
-Vineet
