* [committed] [RISC-V] Avoid sub-word mode comparisons with Zicond
@ 2023-08-02 5:41 Jeff Law
2023-08-05 7:46 ` Xiao Zeng
0 siblings, 1 reply; 3+ messages in thread
From: Jeff Law @ 2023-08-02 5:41 UTC (permalink / raw)
To: gcc-patches; +Cc: Xiao Zeng
[-- Attachment #1: Type: text/plain, Size: 4466 bytes --]
c-torture/execute/pr59014-2.c fails with the Zicond work on rv64. We
miscompile the "foo" routine because we have eliminated a required sign
extension.
The key routine looks like this:
foo (long long int x, long long int y)
{
if (((int) x | (int) y) != 0)
return 6;
return x + y;
}
So we kindof do the expected thing at expansion time. We IOR X and Y,
sign extend the result from 32 to 64 bits (note how the values in the
source are casted from long long to ints), then emit a suitable
conditional branch. ie:
> (insn 10 4 12 2 (set (reg:DI 142)
> (ior:DI (reg/v:DI 138 [ x ])
> (reg/v:DI 139 [ y ]))) "j.c":6:16 99 {iordi3}
> (nil))
> (insn 12 10 13 2 (set (reg:DI 144)
> (sign_extend:DI (subreg:SI (reg:DI 142) 0))) "j.c":6:6 116 {extendsidi2}
> (nil))
> (jump_insn 13 12 14 2 (set (pc)
> (if_then_else (ne (reg:DI 144)
> (const_int 0 [0]))
> (label_ref:DI 27)
> (pc))) "j.c":6:6 243 {*branchdi}
> (expr_list:REG_DEAD (reg:DI 144)
> (int_list:REG_BR_PROB 233216732 (nil)))
When we if-convert that we generate this sequence:
> (insn 10 4 12 2 (set (reg:DI 142)
> (ior:DI (reg/v:DI 138 [ x ])
> (reg/v:DI 139 [ y ]))) "j.c":6:16 99 {iordi3}
> (nil))
> (insn 12 10 30 2 (set (reg:DI 144)
> (sign_extend:DI (subreg:SI (reg:DI 142) 0))) "j.c":6:6 116 {extendsidi2}
> (nil))
> (insn 30 12 31 2 (set (reg:DI 147)
> (const_int 6 [0x6])) "j.c":8:12 179 {*movdi_64bit}
> (nil))
> (insn 31 30 33 2 (set (reg:DI 146)
> (plus:DI (reg/v:DI 138 [ x ])
> (reg/v:DI 139 [ y ]))) "j.c":8:12 5 {adddi3}
> (nil))
> (insn 33 31 34 2 (set (reg:DI 149)
> (if_then_else:DI (ne:DI (reg:DI 144)
> (const_int 0 [0]))
> (const_int 0 [0])
> (reg:DI 146))) "j.c":8:12 11368 {*czero.nez.didi}
> (nil))
> (insn 34 33 35 2 (set (reg:DI 148)
> (if_then_else:DI (eq:DI (reg:DI 144)
> (const_int 0 [0]))
> (const_int 0 [0])
> (reg:DI 147))) "j.c":8:12 11367 {*czero.eqz.didi}
> (nil))
> (insn 35 34 21 2 (set (reg:DI 137 [ <retval> ])
> (ior:DI (reg:DI 148)
> (reg:DI 149))) "j.c":8:12 99 {iordi3}
> (nil))
Which looks basically OK. The sign extended subreg is a bit worrisome
though. And sure enough when we get into combine:
> Failed to match this instruction:
> (parallel [
> (set (reg:DI 149)
> (if_then_else:DI (eq:DI (subreg:SI (reg:DI 142) 0)
> (const_int 0 [0]))
> (reg:DI 146)
> (const_int 0 [0])))
> (set (reg:DI 144)
> (sign_extend:DI (subreg:SI (reg:DI 142) 0)))
> ])
> Successfully matched this instruction:
> (set (reg:DI 144)
> (sign_extend:DI (subreg:SI (reg:DI 142) 0)))
> Successfully matched this instruction:
> (set (reg:DI 149)
> (if_then_else:DI (eq:DI (subreg:SI (reg:DI 142) 0)
> (const_int 0 [0]))
> (reg:DI 146)
> (const_int 0 [0])))
> allowing combination of insns 12 and 33
Since we need the side effect we first try the PARALLEL with two sets.
That, as expected, fails. Generic combine code then tries to pull apart
the two sets as distinct insns resulting in this conditional move:
> (insn 33 31 34 2 (set (reg:DI 149)
> (if_then_else:DI (eq:DI (subreg:SI (reg:DI 142) 0)
> (const_int 0 [0]))
> (reg:DI 146)
> (const_int 0 [0]))) "j.c":8:12 11347 {*czero.nez.disi}
> (expr_list:REG_DEAD (reg:DI 146)
> (nil)))
Bzzt. We can't actually implement this RTL in the hardware. Basically
it's asking to do 32bit comparison on rv64, ignoring the upper 32 bits
of the input register. That's not actually how zicond works.
The operands to the comparison need to be in DImode for rv64 and SImode
for rv32. That's the X iterator. Note the mode of the comparison
operands may be different than the mode of the destination. ie, we
might have a 64bit comparison and produce a 32bit sign extended result
much like the setcc insns support.
This patch changes the 6 zicond patterns to use the X iterator on the
comparison inputs. That at least makes the patterns correct and fixes
this particular testcase. There's a few other lurking problems that
I'll address in additional patches.
Committed to the trunk,
Jeff
[-- Attachment #2: P --]
[-- Type: text/plain, Size: 8890 bytes --]
commit 2d73f2eb80caf328bc4dd1324d475e7bf6b56837
Author: Jeff Law <jlaw@ventanamicro.com>
Date: Tue Aug 1 23:12:16 2023 -0600
[committed] [RISC-V] Avoid sub-word mode comparisons with Zicond
c-torture/execute/pr59014-2.c fails with the Zicond work on rv64. We
miscompile the "foo" routine because we have eliminated a required sign
extension.
The key routine looks like this:
foo (long long int x, long long int y)
{
if (((int) x | (int) y) != 0)
return 6;
return x + y;
}
So we kindof do the expected thing. We IOR X and Y, sign extend the result
from 32 to 64 bits, then emit a suitable conditional branch. ie:
> (insn 10 4 12 2 (set (reg:DI 142)
> (ior:DI (reg/v:DI 138 [ x ])
> (reg/v:DI 139 [ y ]))) "j.c":6:16 99 {iordi3}
> (nil))
> (insn 12 10 13 2 (set (reg:DI 144)
> (sign_extend:DI (subreg:SI (reg:DI 142) 0))) "j.c":6:6 116 {extendsidi2}
> (nil))
> (jump_insn 13 12 14 2 (set (pc)
> (if_then_else (ne (reg:DI 144)
> (const_int 0 [0]))
> (label_ref:DI 27)
> (pc))) "j.c":6:6 243 {*branchdi}
> (expr_list:REG_DEAD (reg:DI 144)
> (int_list:REG_BR_PROB 233216732 (nil)))
When we if-convert that we generate this sequence:
> (insn 10 4 12 2 (set (reg:DI 142)
> (ior:DI (reg/v:DI 138 [ x ])
> (reg/v:DI 139 [ y ]))) "j.c":6:16 99 {iordi3}
> (nil))
> (insn 12 10 30 2 (set (reg:DI 144)
> (sign_extend:DI (subreg:SI (reg:DI 142) 0))) "j.c":6:6 116 {extendsidi2}
> (nil))
> (insn 30 12 31 2 (set (reg:DI 147)
> (const_int 6 [0x6])) "j.c":8:12 179 {*movdi_64bit}
> (nil))
> (insn 31 30 33 2 (set (reg:DI 146)
> (plus:DI (reg/v:DI 138 [ x ])
> (reg/v:DI 139 [ y ]))) "j.c":8:12 5 {adddi3}
> (nil))
> (insn 33 31 34 2 (set (reg:DI 149)
> (if_then_else:DI (ne:DI (reg:DI 144)
> (const_int 0 [0]))
> (const_int 0 [0])
> (reg:DI 146))) "j.c":8:12 11368 {*czero.nez.didi}
> (nil))
> (insn 34 33 35 2 (set (reg:DI 148)
> (if_then_else:DI (eq:DI (reg:DI 144)
> (const_int 0 [0]))
> (const_int 0 [0])
> (reg:DI 147))) "j.c":8:12 11367 {*czero.eqz.didi}
> (nil))
> (insn 35 34 21 2 (set (reg:DI 137 [ <retval> ])
> (ior:DI (reg:DI 148)
> (reg:DI 149))) "j.c":8:12 99 {iordi3}
> (nil))
Which looks basically OK. The sign extended subreg is a bit worrisome though.
And sure enough when we get into combine:
> Failed to match this instruction:
> (parallel [
> (set (reg:DI 149)
> (if_then_else:DI (eq:DI (subreg:SI (reg:DI 142) 0)
> (const_int 0 [0]))
> (reg:DI 146)
> (const_int 0 [0])))
> (set (reg:DI 144)
> (sign_extend:DI (subreg:SI (reg:DI 142) 0)))
> ])
> Successfully matched this instruction:
> (set (reg:DI 144)
> (sign_extend:DI (subreg:SI (reg:DI 142) 0)))
> Successfully matched this instruction:
> (set (reg:DI 149)
> (if_then_else:DI (eq:DI (subreg:SI (reg:DI 142) 0)
> (const_int 0 [0]))
> (reg:DI 146)
> (const_int 0 [0])))
> allowing combination of insns 12 and 33
Since we need the side effect we first try the PARALLEL with two sets.
That, as expected, fails. Generic combine code then tries to pull apart
the two sets as distinct insns resulting in this conditional move:
> (insn 33 31 34 2 (set (reg:DI 149)
> (if_then_else:DI (eq:DI (subreg:SI (reg:DI 142) 0)
> (const_int 0 [0]))
> (reg:DI 146)
> (const_int 0 [0]))) "j.c":8:12 11347 {*czero.nez.disi}
> (expr_list:REG_DEAD (reg:DI 146)
> (nil)))
Bzzt. We can't actually implement this RTL in the hardware. Basically
it's asking to do 32bit comparison on rv64, ignoring the upper 32 bits
of the input register. That's not actually how zicond works.
The operands to the comparison need to be in DImode for rv64 and SImode
for rv32. That's the X iterator. Note the mode of the comparison
operands may be different than the mode of the destination. ie, we might
have a 64bit comparison and produce a 32bit sign extended result much
like the setcc insns support.
This patch changes the 6 zicond patterns to use the X iterator on the
comparison inputs and fixes the testsuite failure.
gcc/
* config/riscv/zicond.md: Use the X iterator instead of ANYI
on the comparison input operands.
diff --git a/gcc/config/riscv/zicond.md b/gcc/config/riscv/zicond.md
index 723a22422e1..8f24b3a1690 100644
--- a/gcc/config/riscv/zicond.md
+++ b/gcc/config/riscv/zicond.md
@@ -22,9 +22,9 @@ (define_code_attr eqz [(eq "nez") (ne "eqz")])
(define_code_attr nez [(eq "eqz") (ne "nez")])
;; Zicond
-(define_insn "*czero.<eqz>.<GPR:mode><ANYI:mode>"
+(define_insn "*czero.<eqz>.<GPR:mode><X:mode>"
[(set (match_operand:GPR 0 "register_operand" "=r")
- (if_then_else:GPR (eq_or_ne (match_operand:ANYI 1 "register_operand" "r")
+ (if_then_else:GPR (eq_or_ne (match_operand:X 1 "register_operand" "r")
(const_int 0))
(match_operand:GPR 2 "register_operand" "r")
(const_int 0)))]
@@ -32,9 +32,9 @@ (define_insn "*czero.<eqz>.<GPR:mode><ANYI:mode>"
"czero.<eqz>\t%0,%2,%1"
)
-(define_insn "*czero.<nez>.<GPR:mode><ANYI:mode>"
+(define_insn "*czero.<nez>.<GPR:mode><X:mode>"
[(set (match_operand:GPR 0 "register_operand" "=r")
- (if_then_else:GPR (eq_or_ne (match_operand:ANYI 1 "register_operand" "r")
+ (if_then_else:GPR (eq_or_ne (match_operand:X 1 "register_operand" "r")
(const_int 0))
(const_int 0)
(match_operand:GPR 2 "register_operand" "r")))]
@@ -43,9 +43,9 @@ (define_insn "*czero.<nez>.<GPR:mode><ANYI:mode>"
)
;; Special optimization under eq/ne in primitive semantics
-(define_insn "*czero.eqz.<GPR:mode><ANYI:mode>.opt1"
+(define_insn "*czero.eqz.<GPR:mode><X:mode>.opt1"
[(set (match_operand:GPR 0 "register_operand" "=r")
- (if_then_else:GPR (eq (match_operand:ANYI 1 "register_operand" "r")
+ (if_then_else:GPR (eq (match_operand:X 1 "register_operand" "r")
(const_int 0))
(match_operand:GPR 2 "register_operand" "1")
(match_operand:GPR 3 "register_operand" "r")))]
@@ -53,9 +53,9 @@ (define_insn "*czero.eqz.<GPR:mode><ANYI:mode>.opt1"
"czero.eqz\t%0,%3,%1"
)
-(define_insn "*czero.eqz.<GPR:mode><ANYI:mode>.opt2"
+(define_insn "*czero.eqz.<GPR:mode><X:mode>.opt2"
[(set (match_operand:GPR 0 "register_operand" "=r")
- (if_then_else:GPR (eq (match_operand:ANYI 1 "register_operand" "r")
+ (if_then_else:GPR (eq (match_operand:X 1 "register_operand" "r")
(const_int 0))
(match_operand:GPR 2 "register_operand" "r")
(match_operand:GPR 3 "register_operand" "1")))]
@@ -63,9 +63,9 @@ (define_insn "*czero.eqz.<GPR:mode><ANYI:mode>.opt2"
"czero.nez\t%0,%2,%1"
)
-(define_insn "*czero.nez.<GPR:mode><ANYI:mode>.opt3"
+(define_insn "*czero.nez.<GPR:mode><X:mode>.opt3"
[(set (match_operand:GPR 0 "register_operand" "=r")
- (if_then_else:GPR (ne (match_operand:ANYI 1 "register_operand" "r")
+ (if_then_else:GPR (ne (match_operand:X 1 "register_operand" "r")
(const_int 0))
(match_operand:GPR 2 "register_operand" "r")
(match_operand:GPR 3 "register_operand" "1")))]
@@ -73,9 +73,9 @@ (define_insn "*czero.nez.<GPR:mode><ANYI:mode>.opt3"
"czero.eqz\t%0,%2,%1"
)
-(define_insn "*czero.nez.<GPR:mode><ANYI:mode>.opt4"
+(define_insn "*czero.nez.<GPR:mode><X:mode>.opt4"
[(set (match_operand:GPR 0 "register_operand" "=r")
- (if_then_else:GPR (ne (match_operand:ANYI 1 "register_operand" "r")
+ (if_then_else:GPR (ne (match_operand:X 1 "register_operand" "r")
(const_int 0))
(match_operand:GPR 2 "register_operand" "1")
(match_operand:GPR 3 "register_operand" "r")))]
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [committed] [RISC-V] Avoid sub-word mode comparisons with Zicond 2023-08-02 5:41 [committed] [RISC-V] Avoid sub-word mode comparisons with Zicond Jeff Law @ 2023-08-05 7:46 ` Xiao Zeng 2023-08-05 14:56 ` Jeff Law 0 siblings, 1 reply; 3+ messages in thread From: Xiao Zeng @ 2023-08-05 7:46 UTC (permalink / raw) To: Jeff Law, gcc-patches; +Cc: gaofei On Wed, Aug 02, 2023 at 01:41:00 PM Jeff Law <jlaw@ventanamicro.com> wrote: > >c-torture/execute/pr59014-2.c fails with the Zicond work on rv64. We >miscompile the "foo" routine because we have eliminated a required sign >extension. > >The key routine looks like this: > >foo (long long int x, long long int y) >{ > if (((int) x | (int) y) != 0) > return 6; > return x + y; >} > >So we kindof do the expected thing at expansion time. We IOR X and Y, >sign extend the result from 32 to 64 bits (note how the values in the >source are casted from long long to ints), then emit a suitable >conditional branch. ie: > >> (insn 10 4 12 2 (set (reg:DI 142) >> (ior:DI (reg/v:DI 138 [ x ]) >> (reg/v:DI 139 [ y ]))) "j.c":6:16 99 {iordi3} >> (nil)) >> (insn 12 10 13 2 (set (reg:DI 144) >> (sign_extend:DI (subreg:SI (reg:DI 142) 0))) "j.c":6:6 116 {extendsidi2} >> (nil)) >> (jump_insn 13 12 14 2 (set (pc) >> (if_then_else (ne (reg:DI 144) >> (const_int 0 [0])) >> (label_ref:DI 27) >> (pc))) "j.c":6:6 243 {*branchdi} >> (expr_list:REG_DEAD (reg:DI 144) >> (int_list:REG_BR_PROB 233216732 (nil))) > >When we if-convert that we generate this sequence: > >> (insn 10 4 12 2 (set (reg:DI 142) >> (ior:DI (reg/v:DI 138 [ x ]) >> (reg/v:DI 139 [ y ]))) "j.c":6:16 99 {iordi3} >> (nil)) >> (insn 12 10 30 2 (set (reg:DI 144) >> (sign_extend:DI (subreg:SI (reg:DI 142) 0))) "j.c":6:6 116 {extendsidi2} >> (nil)) >> (insn 30 12 31 2 (set (reg:DI 147) >> (const_int 6 [0x6])) "j.c":8:12 179 {*movdi_64bit} >> (nil)) >> (insn 31 30 33 2 (set (reg:DI 146) >> (plus:DI (reg/v:DI 138 [ x ]) >> (reg/v:DI 139 [ y ]))) "j.c":8:12 5 {adddi3} >> (nil)) >> (insn 33 31 34 2 (set (reg:DI 149) >> (if_then_else:DI (ne:DI (reg:DI 144) >> (const_int 0 [0])) >> (const_int 0 [0]) >> (reg:DI 146))) "j.c":8:12 11368 {*czero.nez.didi} >> (nil)) >> (insn 34 33 35 2 (set (reg:DI 148) >> (if_then_else:DI (eq:DI (reg:DI 144) >> (const_int 0 [0])) >> (const_int 0 [0]) >> (reg:DI 147))) "j.c":8:12 11367 {*czero.eqz.didi} >> (nil)) >> (insn 35 34 21 2 (set (reg:DI 137 [ <retval> ]) >> (ior:DI (reg:DI 148) >> (reg:DI 149))) "j.c":8:12 99 {iordi3} >> (nil)) > >Which looks basically OK. The sign extended subreg is a bit worrisome >though. And sure enough when we get into combine: > >> Failed to match this instruction: >> (parallel [ >> (set (reg:DI 149) >> (if_then_else:DI (eq:DI (subreg:SI (reg:DI 142) 0) >> (const_int 0 [0])) >> (reg:DI 146) >> (const_int 0 [0]))) >> (set (reg:DI 144) >> (sign_extend:DI (subreg:SI (reg:DI 142) 0))) >> ]) >> Successfully matched this instruction: >> (set (reg:DI 144) >> (sign_extend:DI (subreg:SI (reg:DI 142) 0))) >> Successfully matched this instruction: >> (set (reg:DI 149) >> (if_then_else:DI (eq:DI (subreg:SI (reg:DI 142) 0) >> (const_int 0 [0])) >> (reg:DI 146) >> (const_int 0 [0]))) >> allowing combination of insns 12 and 33 > >Since we need the side effect we first try the PARALLEL with two sets. >That, as expected, fails. Generic combine code then tries to pull apart >the two sets as distinct insns resulting in this conditional move: > >> (insn 33 31 34 2 (set (reg:DI 149) >> (if_then_else:DI (eq:DI (subreg:SI (reg:DI 142) 0) >> (const_int 0 [0])) >> (reg:DI 146) >> (const_int 0 [0]))) "j.c":8:12 11347 {*czero.nez.disi} >> (expr_list:REG_DEAD (reg:DI 146) >> (nil))) > >Bzzt. We can't actually implement this RTL in the hardware. Basically >it's asking to do 32bit comparison on rv64, ignoring the upper 32 bits >of the input register. That's not actually how zicond works. > >The operands to the comparison need to be in DImode for rv64 and SImode >for rv32. That's the X iterator. After analyzing the rtl log, I can't agree more with this sentence. >Note the mode of the comparison >operands may be different than the mode of the destination. ie, we >might have a 64bit comparison and produce a 32bit sign extended result >much like the setcc insns support. > >This patch changes the 6 zicond patterns to use the X iterator on the >comparison inputs. That at least makes the patterns correct and fixes >this particular testcase. There's a few other lurking problems that >I'll address in additional patches. > >Committed to the trunk, >Jeff 1 In any case, jeff's analysis is convincing, here I will add a little bit of my own analysis. 2 First, for the test cases: foo(long long int x, long long int y) { if (((int)x | (int)y) != 0) return 6; return x + y; } look directly at the compared assembly code. This allows people to quickly realize where the error occurred. X_mode.s(right) ANYI_mode.S(error) 10 foo: 10 foo: 11 or a5,a0,a1 11 or a5,a0,a1 12 sext.w a5,a5 12 li a4,6 13 addw a0,a0,a1 13 addw a0,a0,a1 14 li a4,6 15 czero.nez a1,a0,a5 14 czero.nez a1,a0,a5 16 czero.eqz a0,a4,a5 15 czero.eqz a0,a4,a5 17 or a0,a0,a1 16 or a0,a0,a1 18 ret 17 ret 3 You will find that the correct assembly is just one more assembly instruction: sext.w a5,a5, the rest of the two are exactly the same. 4 From the perspective of assembly instructions, the a5 value obtained by sext.w a5, a5 may be different from the original a5 value, which leads to errors in the test case. 5 However, it is difficult to directly see that an error has occurred from the rtl log of gcc's passe. 6 I'm wondering about transforms like this: In test.c.c.301r.ira (insn 36 34 42 2 (set (reg:DI 153) (if_then_else:DI (eq:DI (subreg:SI (reg:DI 145) 0) (const_int 0 [0])) (reg:DI 149) (const_int 0 [0]))) "test.c":4:12 13599 {*czero.nez.disi} (expr_list:REG_DEAD (reg:DI 149) (nil))) In test.c.c.302r.reload it becomes (insn 36 34 42 2 (set (reg:DI 11 a1 [153]) (if_then_else:DI (eq:DI (reg:SI 15 a5 [145]) (const_int 0 [0])) (reg:DI 10 a0 [149]) (const_int 0 [0]))) "test.c":4:12 13599 {*czero.nez.disi} (nil)) Obviously, (subreg:SI (reg:DI 145) 0) is transformed into (reg:SI 15 a5 [145]) after passing through reload pass. This conversion is wrong, why did gcc not warn? 7 I'm not very familiar with reload pass, maybe someone can give me a brief introduction, or tell me where to find relevant information? Thanks. Thanks Xiao Zeng ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [committed] [RISC-V] Avoid sub-word mode comparisons with Zicond 2023-08-05 7:46 ` Xiao Zeng @ 2023-08-05 14:56 ` Jeff Law 0 siblings, 0 replies; 3+ messages in thread From: Jeff Law @ 2023-08-05 14:56 UTC (permalink / raw) To: Xiao Zeng, gcc-patches; +Cc: gaofei On 8/5/23 01:46, Xiao Zeng wrote: >> >> The operands to the comparison need to be in DImode for rv64 and SImode >> for rv32. That's the X iterator. > After analyzing the rtl log, I can't agree more with this sentence. > >> Note the mode of the comparison >> operands may be different than the mode of the destination. ie, we >> might have a 64bit comparison and produce a 32bit sign extended result >> much like the setcc insns support. >> >> This patch changes the 6 zicond patterns to use the X iterator on the >> comparison inputs. That at least makes the patterns correct and fixes >> this particular testcase. There's a few other lurking problems that >> I'll address in additional patches. >> >> Committed to the trunk, >> Jeff > > 1 In any case, jeff's analysis is convincing, here I will add a little bit of my own analysis. > > 2 First, for the test cases: > > foo(long long int x, long long int y) { > if (((int)x | (int)y) != 0) > return 6; > return x + y; > } > > look directly at the compared assembly code. This allows people to quickly > realize where the error occurred. > > X_mode.s(right) ANYI_mode.S(error) > 10 foo: 10 foo: > 11 or a5,a0,a1 11 or a5,a0,a1 > 12 sext.w a5,a5 12 li a4,6 > 13 addw a0,a0,a1 13 addw a0,a0,a1 > 14 li a4,6 > 15 czero.nez a1,a0,a5 14 czero.nez a1,a0,a5 > 16 czero.eqz a0,a4,a5 15 czero.eqz a0,a4,a5 > 17 or a0,a0,a1 16 or a0,a0,a1 > 18 ret 17 ret > > 3 You will find that the correct assembly is just one more assembly > instruction: sext.w a5,a5, the rest of the two are exactly the same. > > 4 From the perspective of assembly instructions, the a5 value obtained > by sext.w a5, a5 may be different from the original a5 value, which leads > to errors in the test case. Exactly. > > 5 However, it is difficult to directly see that an error has occurred > from the rtl log of gcc's passe. Yes. It's pretty subtle, but par for the course once subreg expressions are involved. subregs are a major wart in the design/implementation of RTL due to them having fairly obscure semantics that are dependent on whether they are widening or narrowing subregs, if the inner object is a mem or something else, etc and how they interact with WORD_REGISTER_OPERATIONS. > > 6 I'm wondering about transforms like this: > > In test.c.c.301r.ira > (insn 36 34 42 2 (set (reg:DI 153) > (if_then_else:DI (eq:DI (subreg:SI (reg:DI 145) 0) > (const_int 0 [0])) > (reg:DI 149) > (const_int 0 [0]))) "test.c":4:12 13599 {*czero.nez.disi} > (expr_list:REG_DEAD (reg:DI 149) > (nil))) But it's already broken in this form. There was a sign extension in the IL up through the combine pass. In fact, I nearly made the argument that the bug was in combine's simplification of (sign_extend (subreg ...)) to (subreg ...). But ultimately the hardware semantics of risc-v are that comparisons look at the full register, plain and simple. So a narrowing subreg to a sub-word mode is simply wrong i this context. This is also consistent with the implementation of conditional branches and scc insns in the risc-v backend. > > In test.c.c.302r.reload it becomes > (insn 36 34 42 2 (set (reg:DI 11 a1 [153]) > (if_then_else:DI (eq:DI (reg:SI 15 a5 [145]) > (const_int 0 [0])) > (reg:DI 10 a0 [149]) > (const_int 0 [0]))) "test.c":4:12 13599 {*czero.nez.disi} > (nil)) > > Obviously, (subreg:SI (reg:DI 145) 0) is transformed into (reg:SI 15 a5 [145]) after > passing through reload pass. This conversion is wrong, why did gcc not warn? The only reason subregs exist is because pseudo registers have a single mode -- which derives from the fact that there is precisely once instance of any given pseudo register. See RTL sharing section of the internals manual. A subreg expression allows us to look at a pseudo in a mode other than its natural mode. Essentially its the same bits reinterpreted in another mode. I'm over-simplifying a lot. The full details of subreg semantics are documented in the RTL section of the internals manual. WORD_REGISTER_OPERATIONS is a property of the target that (again over-simplifying) says that an operation such as an add may have a given size (say SImode) in the RTL, but the hardware performs the operation in its native word size (DImode for rv64). While WORD_REGISTER_OPERATIONS has been around since the 90s, its semantics have always been a bit vague. It's seen as a wart and I'd very much like to get away from using it on the RISC-V port. Register allocation assigns hard registers to the pseudo registers and the RTL is changed to reflect that register allocation. At that point we have no more pseudos in the IL as they have been replaced by hard registers. Register allocation is split into two distinct phases. IRA and LRA. It's an over-simplification, but IRA is the main register allocator and LRA is primarily tasked with making each operand match its constraints -- ie spilling and reloading. Hard registers are not shared objects, meaning we can have multiple instances of the same hard register. So near the end of register allocation we transform all subregs into an appropriate reg expression. This simplifies the implementation of various passes that run after register allocation/reloading. So the transformation (subreg:SI (reg:DI a5)) into (reg:SI a5) by the register allocation is valid and normal. That subreg expression should never have shown up in a comparison to begin with. > > 7 I'm not very familiar with reload pass, maybe someone can give me a > brief introduction, or tell me where to find relevant information? Thanks. There is some discussion of reload in the internals manual which covers some of the basics. The implementation details are incredibly complex. Additionally there are two completely distinct implementations. There is "reload" which was the original implementation from the late 1980s and is still used by some targets, though it's considered deprecated at this point. And there's the LRA implementation which was added in the 2000s. The RISC-V port is an LRA port. The best documentation of LRA is the code itself (lra-*.cc) and the papers that have presented at the various GNU Cauldron and GCC Summit conferences. Jeff ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-08-05 14:56 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-08-02 5:41 [committed] [RISC-V] Avoid sub-word mode comparisons with Zicond Jeff Law 2023-08-05 7:46 ` Xiao Zeng 2023-08-05 14:56 ` Jeff Law
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).