Re: [PATCH v2] RISC-V: Add split pattern to generate SFB instructions. [PR113095]

[Monk Chiang]

Thank you for your help. I will update the test case.
I test on the Coremark and have 5% improvement on the SiFive CPU.

On Tue, Jan 23, 2024 at 12:24 PM Jeff Law  wrote:

>
>
> On 1/21/24 23:12, Monk Chiang wrote:
> > Since the match.pd transforms (zero_one == 0) ? y : z  y,
> > into ((typeof(y))zero_one * z)  y. Add splitters to recongize
> > this expression to generate SFB instructions.
> >
> > gcc/ChangeLog:
> >   PR target/113095
> >   * config/riscv/sfb.md: New splitters to rewrite single bit
> >   sign extension as the condition to SFB instructions.
> >
> > gcc/testsuite/ChangeLog:
> >  * gcc.target/riscv/sfb.c: New test.
> >   * gcc.target/riscv/pr113095.c: New test.
> So the 113095 test is going to fail to link on rv64 causing a testsuite
> failure.  I would suggest it have these dg-options lines instead of the
> one you provided:
>
> /* { dg-options "-O2 -march=rv32gc -mabi=ilp32d -mtune=sifive-7-series"
> { target { rv32 } } } */
> /* { dg-options "-O2 -march=rv64gc -mabi=lp64d -mtune=sifive-7-series" {
> target { rv64 } } } */
>
>
> A similar change is not strictly needed for the new sfb.c test since it
> only does a compile (but not a link) test.
>
> You still didn't indicating what testing was done for this patch.
> Standard practice is to build the compiler and run the testsuite with
> and without your change and verify there are no regressions.  Ideally
> new tests should pass as well.
>
> I made the change above locally to pr113095.c to fix those failures on
> rv64.   So this is OK with the adjustment to the dg-options line in the
> new pr113095 test.
>
> Jeff
>
>

Re: [PATCH v2] RISC-V: Add split pattern to generate SFB instructions. [PR113095]

[Jeff Law]

On 1/21/24 23:12, Monk Chiang wrote:

Since the match.pd transforms (zero_one == 0) ? y : z  y,
into ((typeof(y))zero_one * z)  y. Add splitters to recongize
this expression to generate SFB instructions.

gcc/ChangeLog:
PR target/113095
* config/riscv/sfb.md: New splitters to rewrite single bit
sign extension as the condition to SFB instructions.

gcc/testsuite/ChangeLog:
 * gcc.target/riscv/sfb.c: New test.
* gcc.target/riscv/pr113095.c: New test.
So the 113095 test is going to fail to link on rv64 causing a testsuite 
failure.  I would suggest it have these dg-options lines instead of the 
one you provided:


/* { dg-options "-O2 -march=rv32gc -mabi=ilp32d -mtune=sifive-7-series" 
{ target { rv32 } } } */
/* { dg-options "-O2 -march=rv64gc -mabi=lp64d -mtune=sifive-7-series" { 
target { rv64 } } } */



A similar change is not strictly needed for the new sfb.c test since it 
only does a compile (but not a link) test.


You still didn't indicating what testing was done for this patch. 
Standard practice is to build the compiler and run the testsuite with 
and without your change and verify there are no regressions.  Ideally 
new tests should pass as well.


I made the change above locally to pr113095.c to fix those failures on 
rv64.   So this is OK with the adjustment to the dg-options line in the 
new pr113095 test.


Jeff

[PATCH v2] RISC-V: Add split pattern to generate SFB instructions. [PR113095]

[Monk Chiang]

Since the match.pd transforms (zero_one == 0) ? y : z  y,
into ((typeof(y))zero_one * z)  y. Add splitters to recongize
this expression to generate SFB instructions.

gcc/ChangeLog:
PR target/113095
* config/riscv/sfb.md: New splitters to rewrite single bit
sign extension as the condition to SFB instructions.

gcc/testsuite/ChangeLog:
* gcc.target/riscv/sfb.c: New test.
* gcc.target/riscv/pr113095.c: New test.
---
 gcc/config/riscv/sfb.md   | 32 +++
 gcc/testsuite/gcc.target/riscv/pr113095.c | 20 ++
 gcc/testsuite/gcc.target/riscv/sfb.c  | 24 +
 3 files changed, 76 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/riscv/pr113095.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/sfb.c

diff --git a/gcc/config/riscv/sfb.md b/gcc/config/riscv/sfb.md
index 8ab747142c8..bfd229e3d09 100644
--- a/gcc/config/riscv/sfb.md
+++ b/gcc/config/riscv/sfb.md
@@ -35,3 +35,35 @@
   [(set_attr "length" "8")
(set_attr "type" "sfb_alu")
(set_attr "mode" "")])
+
+;; Combine creates this form ((typeof(y))zero_one * z)  y
+;; for SiFive short forward branches.
+
+(define_split
+  [(set (match_operand:X 0 "register_operand")
+   (and:X (sign_extract:X (match_operand:X 1 "register_operand")
+  (const_int 1)
+  (match_operand 2 "immediate_operand"))
+  (match_operand:X 3 "register_operand")))
+   (clobber (match_operand:X 4 "register_operand"))]
+  "TARGET_SFB_ALU"
+  [(set (match_dup 4) (zero_extract:X (match_dup 1) (const_int 1) (match_dup 
2)))
+   (set (match_dup 0) (if_then_else:X (ne (match_dup 4) (const_int 0))
+ (match_dup 3)
+ (const_int 0)))])
+
+(define_split
+  [(set (match_operand:X 0 "register_operand")
+   (and:X (sign_extract:X (match_operand:X 1 "register_operand")
+  (const_int 1)
+  (match_operand 2 "immediate_operand"))
+  (match_operand:X 3 "register_operand")))
+   (clobber (match_operand:X 4 "register_operand"))]
+  "TARGET_SFB_ALU && (UINTVAL (operands[2]) < 11)"
+  [(set (match_dup 4) (and:X (match_dup 1) (match_dup 2)))
+   (set (match_dup 0) (if_then_else:X (ne (match_dup 4) (const_int 0))
+ (match_dup 3)
+ (const_int 0)))]
+{
+  operands[2] = GEN_INT (1 << UINTVAL(operands[2]));
+})
diff --git a/gcc/testsuite/gcc.target/riscv/pr113095.c 
b/gcc/testsuite/gcc.target/riscv/pr113095.c
new file mode 100644
index 000..50dd6be11c4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/pr113095.c
@@ -0,0 +1,20 @@
+/* { dg-do run } */
+/* { dg-options "-O2 -march=rv32gc -mabi=ilp32d -mtune=sifive-7-series" } */
+
+extern void abort (void);
+extern void exit (int);
+
+unsigned short __attribute__ ((noinline, noclone))
+foo (unsigned short x) {
+  if (x == 1)
+x ^= 0x4002;
+
+  return x;
+}
+
+int main () {
+  if (foo(1) != 0x4003)
+abort ();
+
+  exit(0);
+}
diff --git a/gcc/testsuite/gcc.target/riscv/sfb.c 
b/gcc/testsuite/gcc.target/riscv/sfb.c
new file mode 100644
index 000..22f164051f4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/sfb.c
@@ -0,0 +1,24 @@
+//* { dg-do compile } */
+/* { dg-options "-O2 -march=rv32gc -mabi=ilp32d -mtune=sifive-7-series" } */
+
+int f1(unsigned int x, unsigned int y, unsigned int z)
+{
+  return ((x & 1) == 0) ? y : z ^ y;
+}
+
+int f2(unsigned int x, unsigned int y, unsigned int z)
+{
+  return ((x & 1) != 0) ? z ^ y : y;
+}
+
+int f3(unsigned int x, unsigned int y, unsigned int z)
+{
+  return ((x & 1) == 0) ? y : z | y;
+}
+
+int f4(unsigned int x, unsigned int y, unsigned int z)
+{
+  return ((x & 1) != 0) ? z | y : y;
+}
+/* { dg-final { scan-assembler-times "bne" 4 } } */
+/* { dg-final { scan-assembler-times "movcc" 4 } } */
-- 
2.40.1