[to-be-committed,RISC-V,PR,target/116283] Fix split code for recent Zbs improvements with masked bit positions

So Patrick's fuzzer found an interesting little buglet in the Zbs 
improvements I added a couple months back.

Specifically when we have masked bit position for a Zbs instruction.  If 
the mask has extraneous bits set we'll generate an unrecognizable insn 
due to an invalid constant.

More concretely, let's take this pattern:


> (define_insn_and_split ""
>   [(set (match_operand:DI 0 "register_operand" "=r")
>         (any_extend:DI
>          (ashift:SI (const_int 1)
>                     (subreg:QI  
>                       (and:DI (match_operand:DI 1 "register_operand" "r")
>                               (match_operand 2 "const_int_operand")) 0))))]
What we need to know to transform this into bset for rv64.

After masking the shift count we want to know the low 5 bits aren't 
0x1f.  If they were 0x1f, then the constant generated would be 
0x80000000 which would then need sign extension out to 64bits, which the 
bset instruction will not do for us.

We can ignore anything outside the low 5 bits.  The mode of the shift is 
SI, so shifting by 32+ bits is undefined behavior.

It's also worth explicitly mentioning that the hardware is going to mask 
the count against 0x3f.

The net is if (operands[2] & 0x1f) != 0x1f, then this transformation is 
safe.  So onto the generated split code...



>   [(set (match_dup 0) (and:DI (match_dup 1) (match_dup 2)))
>    (set (match_dup 0) (zero_extend:DI (ashift:SI
>                                      (const_int 1)
>                                      (subreg:QI (match_dup 0) 0))))]

Which would seemingly do exactly what we want.   The problem is the 
first split insn.  If the constant does not fit into a simm12, that insn 
won't be recognized resulting in the ICE.

The fix is simple, we just need to mask the constant before generating 
RTL.  We can just mask it against 0x1f since we only care about the low 
5 bits.

This affects multiple patterns.  I've added the appropriate fix to all 
of them.

Tested in my tester.  Waiting for the pre-commit bits to run before pushing.

Jeff
PR target/116283
gcc/
	* config/riscv/bitmanip.md (Zbs combiner patterns/splitters): Mask the
	bit position in the split code appropriately.

gcc/testsuite/

	* gcc.target/riscv/pr116283.c: New test

On 8/9/24 12:31 PM, Jeff Law wrote:
> So Patrick's fuzzer found an interesting little buglet in the Zbs 
> improvements I added a couple months back.
> 
> Specifically when we have masked bit position for a Zbs instruction.  If 
> the mask has extraneous bits set we'll generate an unrecognizable insn 
> due to an invalid constant.
> 
> More concretely, let's take this pattern:
> 
> 
>> (define_insn_and_split ""
>>   [(set (match_operand:DI 0 "register_operand" "=r")
>>         (any_extend:DI
>>          (ashift:SI (const_int 1)
>>                     (subreg:QI                       (and:DI 
>> (match_operand:DI 1 "register_operand" "r")
>>                               (match_operand 2 "const_int_operand")) 
>> 0))))]
> What we need to know to transform this into bset for rv64.
> 
> After masking the shift count we want to know the low 5 bits aren't 
> 0x1f.  If they were 0x1f, then the constant generated would be 
> 0x80000000 which would then need sign extension out to 64bits, which the 
> bset instruction will not do for us.
> 
> We can ignore anything outside the low 5 bits.  The mode of the shift is 
> SI, so shifting by 32+ bits is undefined behavior.
> 
> It's also worth explicitly mentioning that the hardware is going to mask 
> the count against 0x3f.
> 
> The net is if (operands[2] & 0x1f) != 0x1f, then this transformation is 
> safe.  So onto the generated split code...
> 
> 
> 
>>   [(set (match_dup 0) (and:DI (match_dup 1) (match_dup 2)))
>>    (set (match_dup 0) (zero_extend:DI (ashift:SI
>>                                      (const_int 1)
>>                                      (subreg:QI (match_dup 0) 0))))]
> 
> Which would seemingly do exactly what we want.   The problem is the 
> first split insn.  If the constant does not fit into a simm12, that insn 
> won't be recognized resulting in the ICE.
> 
> The fix is simple, we just need to mask the constant before generating 
> RTL.  We can just mask it against 0x1f since we only care about the low 
> 5 bits.
> 
> This affects multiple patterns.  I've added the appropriate fix to all 
> of them.
> 
> Tested in my tester.  Waiting for the pre-commit bits to run before 
> pushing.
Bah.  Sent the older version of the patch without the flags fixed for 
the testsuite.

Attached is what I'm committing shortly.

jeff
PR target/116283
gcc/
	* config/riscv/bitmanip.md (Zbs combiner patterns/splitters): Mask the
	bit position in the split code appropriately.

gcc/testsuite/

	* gcc.target/riscv/pr116283.c: New test

diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
index b19295cd942..6872ee56022 100644
--- a/gcc/config/riscv/bitmanip.md
+++ b/gcc/config/riscv/bitmanip.md
@@ -643,7 +643,10 @@ (define_insn_and_split ""
     (set (match_dup 3) (and:DI (not:DI (match_dup 1)) (match_dup 3)))
     (set (match_dup 0) (zero_extend:DI
 			 (ashift:SI (const_int 1) (match_dup 4))))]
-  { operands[4] = gen_lowpart (QImode, operands[3]); }
+{
+  operands[4] = gen_lowpart (QImode, operands[3]);
+  operands[2] = GEN_INT (INTVAL (operands[2]) & 0x1f);
+}
   [(set_attr "type" "bitmanip")])
 
 (define_insn_and_split ""
@@ -662,7 +665,7 @@ (define_insn_and_split ""
    (set (match_dup 0) (zero_extend:DI (ashift:SI
 				     (const_int 1)
 				     (subreg:QI (match_dup 0) 0))))]
-  { }
+  { operands[2] = GEN_INT (INTVAL (operands[2]) & 0x1f); }
   [(set_attr "type" "bitmanip")])
 
 ;; Similarly two patterns for IOR/XOR generating bset/binv to
@@ -687,7 +690,10 @@ (define_insn_and_split ""
    [(set (match_dup 4) (match_dup 2))
     (set (match_dup 4) (and:DI (not:DI (match_dup 1)) (match_dup 4)))
     (set (match_dup 0) (any_or:DI (ashift:DI (const_int 1) (match_dup 5)) (match_dup 3)))]
-  { operands[5] = gen_lowpart (QImode, operands[4]); }
+{
+  operands[5] = gen_lowpart (QImode, operands[4]);
+  operands[2] = GEN_INT (INTVAL (operands[2]) & 0x1f);
+}
   [(set_attr "type" "bitmanip")])
 
 (define_insn_and_split ""
@@ -708,7 +714,7 @@ (define_insn_and_split ""
   "&& reload_completed"
    [(set (match_dup 4) (and:DI (match_dup 1) (match_dup 2)))
     (set (match_dup 0) (any_or:DI (ashift:DI (const_int 1) (subreg:QI (match_dup 4) 0)) (match_dup 3)))]
-  { }
+  { operands[2] = GEN_INT (INTVAL (operands[2]) & 0x1f); }
   [(set_attr "type" "bitmanip")])
 
 ;; Similarly two patterns for AND generating bclr to
@@ -734,7 +740,10 @@ (define_insn_and_split ""
    [(set (match_dup 4) (match_dup 2))
     (set (match_dup 4) (and:DI (not:DI (match_dup 1)) (match_dup 4)))
     (set (match_dup 0) (and:DI (rotate:DI (const_int -2) (match_dup 5)) (match_dup 3)))]
-  { operands[5] = gen_lowpart (QImode, operands[4]); }
+{
+  operands[5] = gen_lowpart (QImode, operands[4]);
+  operands[2] = GEN_INT (INTVAL (operands[2]) & 0x1f);
+}
   [(set_attr "type" "bitmanip")])
 
 
@@ -757,7 +766,10 @@ (define_insn_and_split ""
   "&& reload_completed"
    [(set (match_dup 4) (and:DI (match_dup 1) (match_dup 2)))
     (set (match_dup 0) (and:DI (rotate:DI (const_int -2) (match_dup 5)) (match_dup 3)))]
-  { operands[5] = gen_lowpart (QImode, operands[4]); }
+{
+  operands[5] = gen_lowpart (QImode, operands[4]);
+  operands[2] = GEN_INT (INTVAL (operands[2]) & 0x1f);
+}
   [(set_attr "type" "bitmanip")])
 
 (define_insn "*bset<mode>_1_mask"
diff --git a/gcc/testsuite/gcc.target/riscv/pr116283.c b/gcc/testsuite/gcc.target/riscv/pr116283.c
new file mode 100644
index 00000000000..21861557edc
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/pr116283.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-std=gnu99 -w -march=rv64id_zbs -mabi=lp64d" } */
+
+char c;
+#define d(a, b)                                                                \
+  {                                                                            \
+    __typeof__(a) e = a;                                                       \
+    e;                                                                         \
+  }
+long f;
+void g(signed h[][9][9][9][9]) {
+  for (unsigned i = f; i; i += 3)
+    c = (d(1 << (3629 & h[i][i][1][5][i]), ));
+}
+