So between Milan's work and my own realization earlier this week, we can
finally get resolution on pr123884 and pr106585.
The core issue is for what appear to be natural looking single bit
manipulations we often struggle to generate the bset, bclr or binv on
riscv64.
A few items get in the way. First, rv64 (of course) promotes sub-word
objects (ie ints) into word sized objects. Second, we do have *some*
instructions that work on sub-objects (such as shifts which sign extend
out to 64 bits). Third, a 32-bit object on rv64 is supposed to be sign
extended out to 64 bits when it "escapes" or gets used in a comparison.
Finally, the Zbs instructions do not have forms which sign extend out to
64 bits if the SI sign bit is changed.
Consider this reduced fragment from x264:
int x264_macroblock_encode_p8x8(int dst, int a)
{
dst &= ~(1 << a);
return dst;
}
The code going into ext-dce looks like this:
(insn 7 4 8 2 (set (reg:SI 141)
(const_int 1 [0x1])) "j.c":3:16 276 {*movsi_internal}
(nil))
(insn 8 7 10 2 (set (reg:DI 142)
(sign_extend:DI (ashift:SI (reg:SI 141)
(subreg:QI (reg/v:DI 138 [ a ]) 0)))) "j.c":3:16 312
{ashlsi3_extend}
(expr_list:REG_DEAD (reg:SI 141)
(expr_list:REG_DEAD (reg/v:DI 138 [ a ])
(expr_list:REG_EQUAL (sign_extend:DI (ashift:SI (const_int
1 [0x1])
(subreg:QI (reg/v:DI 138 [ a ]) 0)))
(nil)))))
(insn 10 8 12 2 (set (reg:DI 143)
(not:DI (reg:DI 142))) "j.c":3:12 113 {one_cmpldi2}
(expr_list:REG_DEAD (reg:DI 142)
(nil)))
(insn 12 10 13 2 (set (reg:DI 145)
(and:DI (reg/v:DI 137 [ dst ])
(reg:DI 143))) "j.c":3:9 104 {*anddi3}
(expr_list:REG_DEAD (reg:DI 143)
(expr_list:REG_DEAD (reg/v:DI 137 [ dst ])
(nil))))
(insn 13 12 18 2 (set (reg:DI 146)
(sign_extend:DI (subreg:SI (reg:DI 145) 0))) "j.c":4:12
discrim 1 125 {*extendsidi2_internal}
(expr_list:REG_DEAD (reg:DI 145)
(nil)))
That'll end up generating something like:
li a5,1 # 7 [c=4 l=4] *movsi_internal/1
sllw a1,a5,a1 # 8 [c=8 l=4] ashlsi3_extend
not a1,a1 # 10 [c=4 l=4] one_cmpldi2
and a0,a0,a1 # 18 [c=4 l=4] *anddi3/0
That seems almost tailor made for bclr. Except for the possibility that
we're clearing bit 31. THe RTL above would clear bits 31..63. bclr
just clears one bit.
ext-dce comes along and realizes that insn 13 is redundant and removes
it. But we still have the problem that the RTL would clear bits 31..63
if shift count in (reg:DI 138) is 31 and thus trying to combine the
result and generate a bclr is wrong.
We had the idea that we could follow the bclr with a sign extension.
That works for this testcase, but is wrong in general.
The insight from earlier this week is the semantics of bclr+sext work
when (reg:DI 137) has 33 or more sign bit copies. In that limited, but
common, case the semantics of bclr+sext match the RTL exactly. Just as
important the output is 2 insns, so we can model it as a simple
define_split rather than a define_insn_and_split.
So I've adjusted Milan's patch to check the number of sign bit copies in
the object where we want to clear a single bit. If the number of sign
bit copies is 33 or more, then we allow the splitter to trigger. The
net is we get:
bclr a0,a0,a1 # 10 [c=4 l=4] *bclrdi
sext.w a0,a0 # 18 [c=4 l=4] *extendsidi2_internal/0
The same idea works for bset and binv.
This has been bootstrapped and regression tested on both the c920 (where
it should do nothing, no Zbs extension) and the K3 (where I turn on Zbs
by default). It's also been regression tested on riscv32-elf and
riscv64-elf. I'll obviously wait for pre-commit CI to run before moving
forward. But I think we've finally got this issue nailed down.
jeff
diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
index bd2aaf0587eb..760a8ff669e7 100644
--- a/gcc/config/riscv/bitmanip.md
+++ b/gcc/config/riscv/bitmanip.md
@@ -783,6 +783,49 @@ (define_insn_and_split ""
{ operands[2] = GEN_INT (INTVAL (operands[2]) & 0x1f); }
[(set_attr "type" "bitmanip")])
+;; The goal here is to utilize the Zbs extension on SImode values when doing
+;; so is safe. The problem is if we change the SI sign bit, the sign bit
+;; does not propagate to bits 32..63. That makes preserving semantics of
+;; this kind of RTL harder and more generally makes using the Zbs extension
+;; harder.
+;;
+;; However, if we know the *other* operand has at least 33 sign bit copies,
+;; then we can use the Zbs intruction followed by an sign extension. So the
+;; result is a 3->2 split and we'll still have a chance to eliminate the
+;; trailing sign extension.
+(define_split
+ [(set (match_operand:DI 0 "register_operand")
+ (any_or:DI
+ (sign_extend:DI
+ (ashift:SI
+ (const_int 1)
+ (match_operand:QI 1 "register_operand")))
+ (match_operand:DI 2 "register_operand")))]
+ "(TARGET_64BIT
+ && TARGET_ZBS
+ && num_sign_bit_copies (operands[2], DImode) >= 33)"
+ [(set (match_dup 0) (any_or:DI (ashift:DI (const_int 1) (match_dup 1))
+ (match_dup 2)))
+ (set (match_dup 0)
+ (sign_extend:DI (subreg:SI (match_dup 0) 0)))])
+
+(define_split
+ [(set (match_operand:DI 0 "register_operand")
+ (and:DI (not:DI
+ (sign_extend:DI
+ (ashift:SI
+ (const_int 1)
+ (match_operand:QI 1 "register_operand"))))
+ (match_operand:DI 2 "register_operand")))]
+ "(TARGET_64BIT
+ && TARGET_ZBS
+ && num_sign_bit_copies (operands[2], DImode) >= 33)"
+ [(set (match_dup 0) (and:DI (rotate:DI (const_int -2) (match_dup 1))
+ (match_dup 2)))
+ (set (match_dup 0)
+ (sign_extend:DI (subreg:SI (match_dup 0) 0)))])
+
+
;; Similarly two patterns for AND generating bclr to
;; manipulate a bit in a register
(define_insn_and_split ""
diff --git a/gcc/testsuite/gcc.target/riscv/pr123884-a.c
b/gcc/testsuite/gcc.target/riscv/pr123884-a.c
new file mode 100644
index 000000000000..fb36da04052e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/pr123884-a.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc_zbs -mabi=lp64d" { target rv64 } } */
+/* { dg-options "-march=rv32gc_zbs -mabi=ilp32" { target rv32 } } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og"} } */
+
+int foo(int dst, int a)
+{
+ dst ^= (1 << a);
+ return dst;
+}
+
+/* { dg-final { scan-assembler-times "binv" 1 } } */
+/* { dg-final { scan-assembler-times "sext.w" 1 { target rv64 } } } */
diff --git a/gcc/testsuite/gcc.target/riscv/pr123884-b.c
b/gcc/testsuite/gcc.target/riscv/pr123884-b.c
new file mode 100644
index 000000000000..8ea33cbb32dd
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/pr123884-b.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc_zbs -mabi=lp64d" { target rv64 } } */
+/* { dg-options "-march=rv32gc_zbs -mabi=ilp32" { target rv32 } } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" "-O1" } } */
+
+int x264_macroblock_encode_p8x8(int dst, int a)
+{
+ dst &= ~(1 << a);
+ return dst;
+}
+
+/* { dg-final { scan-assembler-times "bclr" 1 } } */
+/* { dg-final { scan-assembler-times "sext.w" 1 { target rv64 } } } */
+
diff --git a/gcc/testsuite/gcc.target/riscv/pr123884-c.c
b/gcc/testsuite/gcc.target/riscv/pr123884-c.c
new file mode 100644
index 000000000000..10f42a305457
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/pr123884-c.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc_zbs -mabi=lp64d" { target rv64 } } */
+/* { dg-options "-march=rv32gc_zbs -mabi=ilp32" { target rv32 } } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og"} } */
+
+int foo2(int dst, int a)
+{
+ dst |= (1 << a);
+ return dst;
+}
+
+/* { dg-final { scan-assembler-times "bset" 1 } } */
+/* { dg-final { scan-assembler-times "sext.w" 1 { target rv64} } } */
