[committed] RISC-V: Fix INSN costing and more zicond tests

[committed] RISC-V: Fix INSN costing and more zicond tests

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 1209 bytes --]


So this ends up looking a lot like the bits that I had to revert several 
weeks ago :-)

The core issue we have is given an INSN the generic code will cost the 
SET_SRC and SET_DEST and sum them.  But that's far from ideal on a RISC 
target.

For a register destination, the cost can be determined be looking at 
just the SET_SRC.  Which is precisely what this patch does.  When the 
outer code is an INSN and we're presented with a SET we take one of two 
paths.

If the destination is a register, then we recurse just on the SET_SRC 
and we're done.  Otherwise we fall back to the existing code which sums 
the cost of the SET_SRC and SET_DEST.  That fallback path isn't great 
and probably could be further improved (just costing SET_DEST in that 
case is probably quite reasonable).

The difference between this version and the bits that slipped through by 
accident several weeks ago is that old version mis-used the API due to a 
thinko on my part.

This tightens up various zicond tests to avoid undesirable matching.

This has been tested on rv64gc -- the only difference it makes on the 
testsuite is the new tests (included in this patch) flip from failing to 
passing.

Pushed to the trunk.

Jeff

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 30756 bytes --]

commit 44efc743acc01354b6b9eb1939aedfdcc44e71f3
Author: Xiao Zeng <zengxiao@eswincomputing.com>
Date:   Fri Sep 29 16:29:02 2023 -0600

    Fix INSN costing and more zicond tests
    
    So this ends up looking a lot like the bits that I had to revert several weeks
    ago :-)
    
    The core issue we have is given an INSN the generic code will cost the SET_SRC
    and SET_DEST and sum them.  But that's far from ideal on a RISC target.
    
    For a register destination, the cost can be determined be looking at just the
    SET_SRC.  Which is precisely what this patch does.  When the outer code is an
    INSN and we're presented with a SET we take one of two paths.
    
    If the destination is a register, then we recurse just on the SET_SRC and we're
    done.  Otherwise we fall back to the existing code which sums the cost of the
    SET_SRC and SET_DEST.  That fallback path isn't great and probably could be
    further improved (just costing SET_DEST in that case is probably quite
    reasonable).
    
    The difference between this version and the bits that slipped through by
    accident several weeks ago is that old version mis-used the API due to a thinko
    on my part.
    
    This tightens up various zicond tests to avoid undesirable matching.
    
    This has been tested on rv64gc -- the only difference it makes on the testsuite
    is the new tests (included in this patch) flip from failing to passing.
    
    Pushed to the trunk.
    
    gcc/
            * config/riscv/riscv.cc (riscv_rtx_costs): Better handle costing
            SETs when the outer code is INSN.
    
    gcc/testsuite
            * gcc.target/riscv/zicond-primitiveSemantics_compare_imm.c: New test.
            * gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_0_imm.c:
            Likewise.
            * gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_imm.c:
            Likewise.
            * gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_reg.c:
            Likewise.
            * gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_reg_reg.c:
            Likewise.
            * gcc.target/riscv/zicond-primitiveSemantics_compare_reg.c: Likewise.
            * gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_0_imm.c:
            Likewise.
            * gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_imm.c:
            Likewise.
            * gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_reg.c:
            Likewise.
            * gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_reg_reg.c:
            Likewise.
            * gcc.target/riscv/zicond-primitiveSemantics.c: Tighten expected regexp.
            * gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c: Likewise.
            * gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c: Likewise.
            * gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c: Likewise.
            * gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c: Likewise.
            * gcc.target/riscv/zicond-xor-01.c: Likewise.

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 6e7a719e7a0..d5446b63dbf 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -2768,6 +2768,19 @@ riscv_rtx_costs (rtx x, machine_mode mode, int outer_code, int opno ATTRIBUTE_UN
 
   switch (GET_CODE (x))
     {
+    case SET:
+      /* If we are called for an INSN that's a simple set of a register,
+	 then cost based on the SET_SRC alone.  */
+      if (outer_code == INSN && REG_P (SET_DEST (x)))
+	{
+	  riscv_rtx_costs (SET_SRC (x), mode, outer_code, opno, total, speed);
+	  return true;
+	}
+
+      /* Otherwise return FALSE indicating we should recurse into both the
+	 SET_DEST and SET_SRC combining the cost of both.  */
+      return false;
+
     case CONST_INT:
       /* trivial constants checked using OUTER_CODE in case they are
 	 encodable in insn itself w/o need for additional insn(s).  */
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics.c b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics.c
index bcfa04bef91..276dac70852 100644
--- a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics.c
+++ b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics.c
@@ -45,5 +45,5 @@ int primitiveSemantics_11(int a, int b) {
 
 /* { dg-final { scan-assembler-times {\mczero\.eqz\M} 6 } } */
 /* { dg-final { scan-assembler-times {\mczero\.nez\M} 6 } } */
-/* { dg-final { scan-assembler-not {\mbeq} } } */
-/* { dg-final { scan-assembler-not {\mbne} } } */
+/* { dg-final { scan-assembler-not {\mbeq\M} } } */
+/* { dg-final { scan-assembler-not {\mbne\M} } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm.c b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm.c
new file mode 100644
index 00000000000..a53a908ff25
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm.c
@@ -0,0 +1,57 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f" { target { rv32 } } } */
+/* { dg-skip-if "" { *-*-* } {"-O0" "-Og" "-Os" "-Oz"} } */
+
+long primitiveSemantics_compare_imm_00(long a, long b) {
+  return a == 2 ? 0 : b;
+}
+
+long primitiveSemantics_compare_imm_01(long a, long b) {
+  return a != 2 ? 0 : b;
+}
+
+long primitiveSemantics_compare_imm_02(long a, long b) {
+  return a == 2 ? b : 0;
+}
+
+long primitiveSemantics_compare_imm_03(long a, long b) {
+  return a != 2 ? b : 0;
+}
+
+long primitiveSemantics_compare_imm_04(long a, long b) {
+  if (a == 2)
+    b = 0;
+  return b;
+}
+
+long primitiveSemantics_compare_imm_05(long a, long b) {
+  if (!(a == 2))
+    b = 0;
+  return b;
+}
+
+int primitiveSemantics_compare_imm_06(int a, int b) { return a == 2 ? 0 : b; }
+
+int primitiveSemantics_compare_imm_07(int a, int b) { return a != 2 ? 0 : b; }
+
+int primitiveSemantics_compare_imm_08(int a, int b) { return a == 2 ? b : 0; }
+
+int primitiveSemantics_compare_imm_09(int a, int b) { return a != 2 ? b : 0; }
+
+int primitiveSemantics_compare_imm_10(int a, int b) {
+  if ((a == 2))
+    b = 0;
+  return b;
+}
+
+int primitiveSemantics_compare_imm_11(int a, int b) {
+  if (!(a == 2))
+    b = 0;
+  return b;
+}
+
+/* { dg-final { scan-assembler-times {\mczero.eqz\M} 6 } } */
+/* { dg-final { scan-assembler-times {\mczero.nez\M} 6 } } */
+/* { dg-final { scan-assembler-not {\mbeq\M} } } */
+/* { dg-final { scan-assembler-not {\mbne\M} } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_0_imm.c b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_0_imm.c
new file mode 100644
index 00000000000..c90ed100b30
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_0_imm.c
@@ -0,0 +1,73 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f" { target { rv32 } } } */
+/* { dg-skip-if "" { *-*-* } {"-O0" "-Og" "-Os" "-Oz"} } */
+
+long primitiveSemantics_compare_imm_return_0_imm_00(long a, long b) {
+  return a == 2 ? 0 : 5;
+}
+
+long primitiveSemantics_compare_imm_return_0_imm_01(long a, long b) {
+  return a != 2 ? 0 : 5;
+}
+
+long primitiveSemantics_compare_imm_return_0_imm_02(long a, long b) {
+  return a == 2 ? 5 : 0;
+}
+
+long primitiveSemantics_compare_imm_return_0_imm_03(long a, long b) {
+  return a != 2 ? 5 : 0;
+}
+
+long primitiveSemantics_compare_imm_return_0_imm_04(long a, long b) {
+  if (a == 2)
+    b = 0;
+  else
+    b = 5;
+  return b;
+}
+
+long primitiveSemantics_compare_imm_return_0_imm_05(long a, long b) {
+  if (!(a == 2))
+    b = 0;
+  else
+    b = 5;
+  return b;
+}
+
+int primitiveSemantics_compare_imm_return_0_imm_06(int a, int b) {
+  return a == 2 ? 0 : 5;
+}
+
+int primitiveSemantics_compare_imm_return_0_imm_07(int a, int b) {
+  return a != 2 ? 0 : 5;
+}
+
+int primitiveSemantics_compare_imm_return_0_imm_08(int a, int b) {
+  return a == 2 ? 5 : 0;
+}
+
+int primitiveSemantics_compare_imm_return_0_imm_09(int a, int b) {
+  return a != 2 ? 5 : 0;
+}
+
+int primitiveSemantics_compare_imm_return_0_imm_10(int a, int b) {
+  if ((a == 2))
+    b = 0;
+  else
+    b = 5;
+  return b;
+}
+
+int primitiveSemantics_compare_imm_return_0_imm_11(int a, int b) {
+  if (!(a == 2))
+    b = 0;
+  else
+    b = 5;
+  return b;
+}
+
+/* { dg-final { scan-assembler-times {\mczero.eqz\M} 6 } } */
+/* { dg-final { scan-assembler-times {\mczero.nez\M} 6 } } */
+/* { dg-final { scan-assembler-not {\mbeq\M} } } */
+/* { dg-final { scan-assembler-not {\mbne\M} } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_imm.c b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_imm.c
new file mode 100644
index 00000000000..e806f6f0807
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_imm.c
@@ -0,0 +1,73 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f" { target { rv32 } } } */
+/* { dg-skip-if "" { *-*-* } {"-O0" "-Og" "-Os" "-Oz"} } */
+
+long primitiveSemantics_compare_imm_return_imm_imm_00(long a, long b) {
+  return a == 2 ? 7 : 4;
+}
+
+long primitiveSemantics_compare_imm_return_imm_imm_01(long a, long b) {
+  return a != 2 ? 7 : 4;
+}
+
+long primitiveSemantics_compare_imm_return_imm_imm_02(long a, long b) {
+  return a == 2 ? 7 : 4;
+}
+
+long primitiveSemantics_compare_imm_return_imm_imm_03(long a, long b) {
+  return a != 2 ? 7 : 4;
+}
+
+long primitiveSemantics_compare_imm_return_imm_imm_04(long a, long b) {
+  if (a == 2)
+    b = 7;
+  else
+    b = 4;
+  return b;
+}
+
+long primitiveSemantics_compare_imm_return_imm_imm_05(long a, long b) {
+  if (!(a == 2))
+    b = 7;
+  else
+    b = 4;
+  return b;
+}
+
+int primitiveSemantics_compare_imm_return_imm_imm_06(int a, int b) {
+  return a == 2 ? 7 : 4;
+}
+
+int primitiveSemantics_compare_imm_return_imm_imm_07(int a, int b) {
+  return a != 2 ? 7 : 4;
+}
+
+int primitiveSemantics_compare_imm_return_imm_imm_08(int a, int b) {
+  return a == 2 ? 7 : 4;
+}
+
+int primitiveSemantics_compare_imm_return_imm_imm_09(int a, int b) {
+  return a != 2 ? 7 : 4;
+}
+
+int primitiveSemantics_compare_imm_return_imm_imm_10(int a, int b) {
+  if ((a == 2))
+    b = 7;
+  else
+    b = 4;
+  return b;
+}
+
+int primitiveSemantics_compare_imm_return_imm_imm_11(int a, int b) {
+  if (!(a == 2))
+    b = 7;
+  else
+    b = 4;
+  return b;
+}
+
+/* { dg-final { scan-assembler-times {\mczero.eqz\M} 6 } } */
+/* { dg-final { scan-assembler-times {\mczero.nez\M} 6 } } */
+/* { dg-final { scan-assembler-not {\mbeq\M} } } */
+/* { dg-final { scan-assembler-not {\mbne\M} } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_reg.c b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_reg.c
new file mode 100644
index 00000000000..f976d608a03
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_reg.c
@@ -0,0 +1,65 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f" { target { rv32 } } } */
+/* { dg-skip-if "" { *-*-* } {"-O0" "-Og" "-Os" "-Oz"} } */
+
+long primitiveSemantics_compare_imm_return_imm_reg_00(long a, long b) {
+  return a == 2 ? 3 : b;
+}
+
+long primitiveSemantics_compare_imm_return_imm_reg_01(long a, long b) {
+  return a != 2 ? 3 : b;
+}
+
+long primitiveSemantics_compare_imm_return_imm_reg_02(long a, long b) {
+  return a == 2 ? b : 3;
+}
+
+long primitiveSemantics_compare_imm_return_imm_reg_03(long a, long b) {
+  return a != 2 ? b : 3;
+}
+
+long primitiveSemantics_compare_imm_return_imm_reg_04(long a, long b) {
+  if (a == 2)
+    b = 3;
+  return b;
+}
+
+long primitiveSemantics_compare_imm_return_imm_reg_05(long a, long b) {
+  if (!(a == 2))
+    b = 3;
+  return b;
+}
+
+int primitiveSemantics_compare_imm_return_imm_reg_06(int a, int b) {
+  return a == 2 ? 3 : b;
+}
+
+int primitiveSemantics_compare_imm_return_imm_reg_07(int a, int b) {
+  return a != 2 ? 3 : b;
+}
+
+int primitiveSemantics_compare_imm_return_imm_reg_08(int a, int b) {
+  return a == 2 ? b : 3;
+}
+
+int primitiveSemantics_compare_imm_return_imm_reg_09(int a, int b) {
+  return a != 2 ? b : 3;
+}
+
+int primitiveSemantics_compare_imm_return_imm_reg_10(int a, int b) {
+  if ((a == 2))
+    b = 3;
+  return b;
+}
+
+int primitiveSemantics_compare_imm_return_imm_reg_11(int a, int b) {
+  if (!(a == 2))
+    b = 3;
+  return b;
+}
+
+/* { dg-final { scan-assembler-times {\mczero.eqz\M} 6 } } */
+/* { dg-final { scan-assembler-times {\mczero.nez\M} 6 } } */
+/* { dg-final { scan-assembler-not {\mbeq\M} } } */
+/* { dg-final { scan-assembler-not {\mbne\M} } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_reg_reg.c b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_reg_reg.c
new file mode 100644
index 00000000000..90e91192373
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_reg_reg.c
@@ -0,0 +1,65 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f" { target { rv32 } } } */
+/* { dg-skip-if "" { *-*-* } {"-O0" "-Og" "-Os" "-Oz"} } */
+
+long primitiveSemantics_compare_imm_return_reg_reg_00(long a, long b, long c) {
+  return a == 2 ? c : b;
+}
+
+long primitiveSemantics_compare_imm_return_reg_reg_01(long a, long b, long c) {
+  return a != 2 ? c : b;
+}
+
+long primitiveSemantics_compare_imm_return_reg_reg_02(long a, long b, long c) {
+  return a == 2 ? b : c;
+}
+
+long primitiveSemantics_compare_imm_return_reg_reg_03(long a, long b, long c) {
+  return a != 2 ? b : c;
+}
+
+long primitiveSemantics_compare_imm_return_reg_reg_04(long a, long b, long c) {
+  if (a == 2)
+    b = c;
+  return b;
+}
+
+long primitiveSemantics_compare_imm_return_reg_reg_05(long a, long b, long c) {
+  if (!(a == 2))
+    b = c;
+  return b;
+}
+
+int primitiveSemantics_compare_imm_return_reg_reg_06(int a, int b, int c) {
+  return a == 2 ? c : b;
+}
+
+int primitiveSemantics_compare_imm_return_reg_reg_07(int a, int b, int c) {
+  return a != 2 ? c : b;
+}
+
+int primitiveSemantics_compare_imm_return_reg_reg_08(int a, int b, int c) {
+  return a == 2 ? b : c;
+}
+
+int primitiveSemantics_compare_imm_return_reg_reg_09(int a, int b, int c) {
+  return a != 2 ? b : c;
+}
+
+int primitiveSemantics_compare_imm_return_reg_reg_10(int a, int b, int c) {
+  if ((a == 2))
+    b = c;
+  return b;
+}
+
+int primitiveSemantics_compare_imm_return_reg_reg_11(int a, int b, int c) {
+  if (!(a == 2))
+    b = c;
+  return b;
+}
+
+/* { dg-final { scan-assembler-times {\mczero.eqz\M} 12 } } */
+/* { dg-final { scan-assembler-times {\mczero.nez\M} 12 } } */
+/* { dg-final { scan-assembler-not {\mbeq\M} } } */
+/* { dg-final { scan-assembler-not {\mbne\M} } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg.c b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg.c
new file mode 100644
index 00000000000..bfe8c06e1a9
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg.c
@@ -0,0 +1,65 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f" { target { rv32 } } } */
+/* { dg-skip-if "" { *-*-* } {"-O0" "-Og" "-Os" "-Oz"} } */
+
+long primitiveSemantics_compare_reg_00(long a, long b, long c) {
+  return a == c ? 0 : b;
+}
+
+long primitiveSemantics_compare_reg_01(long a, long b, long c) {
+  return a != c ? 0 : b;
+}
+
+long primitiveSemantics_compare_reg_02(long a, long b, long c) {
+  return a == c ? b : 0;
+}
+
+long primitiveSemantics_compare_reg_03(long a, long b, long c) {
+  return a != c ? b : 0;
+}
+
+long primitiveSemantics_compare_reg_04(long a, long b, long c) {
+  if (a == c)
+    b = 0;
+  return b;
+}
+
+long primitiveSemantics_compare_reg_05(long a, long b, long c) {
+  if (!(a == c))
+    b = 0;
+  return b;
+}
+
+int primitiveSemantics_compare_reg_06(int a, int b, int c) {
+  return a == c ? 0 : b;
+}
+
+int primitiveSemantics_compare_reg_07(int a, int b, int c) {
+  return a != c ? 0 : b;
+}
+
+int primitiveSemantics_compare_reg_08(int a, int b, int c) {
+  return a == c ? b : 0;
+}
+
+int primitiveSemantics_compare_reg_09(int a, int b, int c) {
+  return a != c ? b : 0;
+}
+
+int primitiveSemantics_compare_reg_10(int a, int b, int c) {
+  if ((a == c))
+    b = 0;
+  return b;
+}
+
+int primitiveSemantics_compare_reg_11(int a, int b, int c) {
+  if (!(a == c))
+    b = 0;
+  return b;
+}
+
+/* { dg-final { scan-assembler-times {\mczero.eqz\M} 6 } } */
+/* { dg-final { scan-assembler-times {\mczero.nez\M} 6 } } */
+/* { dg-final { scan-assembler-not {\mbeq\M} } } */
+/* { dg-final { scan-assembler-not {\mbne\M} } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_0_imm.c b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_0_imm.c
new file mode 100644
index 00000000000..164de069539
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_0_imm.c
@@ -0,0 +1,73 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f" { target { rv32 } } } */
+/* { dg-skip-if "" { *-*-* } {"-O0" "-Og" "-Os" "-Oz"} } */
+
+long primitiveSemantics_compare_reg_return_0_imm_00(long a, long b, long c) {
+  return a == c ? 0 : 9;
+}
+
+long primitiveSemantics_compare_reg_return_0_imm_01(long a, long b, long c) {
+  return a != c ? 0 : 9;
+}
+
+long primitiveSemantics_compare_reg_return_0_imm_02(long a, long b, long c) {
+  return a == c ? 9 : 0;
+}
+
+long primitiveSemantics_compare_reg_return_0_imm_03(long a, long b, long c) {
+  return a != c ? 9 : 0;
+}
+
+long primitiveSemantics_compare_reg_return_0_imm_04(long a, long b, long c) {
+  if (a == c)
+    b = 0;
+  else
+    b = 9;
+  return b;
+}
+
+long primitiveSemantics_compare_reg_return_0_imm_05(long a, long b, long c) {
+  if (!(a == c))
+    b = 0;
+  else
+    b = 9;
+  return b;
+}
+
+int primitiveSemantics_compare_reg_return_0_imm_06(int a, int b, int c) {
+  return a == c ? 0 : 9;
+}
+
+int primitiveSemantics_compare_reg_return_0_imm_07(int a, int b, int c) {
+  return a != c ? 0 : 9;
+}
+
+int primitiveSemantics_compare_reg_return_0_imm_08(int a, int b, int c) {
+  return a == c ? 9 : 0;
+}
+
+int primitiveSemantics_compare_reg_return_0_imm_09(int a, int b, int c) {
+  return a != c ? 9 : 0;
+}
+
+int primitiveSemantics_compare_reg_return_0_imm_10(int a, int b, int c) {
+  if ((a == c))
+    b = 0;
+  else
+    b = 9;
+  return b;
+}
+
+int primitiveSemantics_compare_reg_return_0_imm_11(int a, int b, int c) {
+  if (!(a == c))
+    b = 0;
+  else
+    b = 9;
+  return b;
+}
+
+/* { dg-final { scan-assembler-times {\mczero.eqz\M} 6 } } */
+/* { dg-final { scan-assembler-times {\mczero.nez\M} 6 } } */
+/* { dg-final { scan-assembler-not {\mbeq\M} } } */
+/* { dg-final { scan-assembler-not {\mbne\M} } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_imm.c b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_imm.c
new file mode 100644
index 00000000000..8ad97abc320
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_imm.c
@@ -0,0 +1,73 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f" { target { rv32 } } } */
+/* { dg-skip-if "" { *-*-* } {"-O0" "-Og" "-Os" "-Oz"} } */
+
+long primitiveSemantics_compare_reg_return_imm_imm_00(long a, long b, long c) {
+  return a == c ? 7 : 4;
+}
+
+long primitiveSemantics_compare_reg_return_imm_imm_01(long a, long b, long c) {
+  return a != c ? 7 : 4;
+}
+
+long primitiveSemantics_compare_reg_return_imm_imm_02(long a, long b, long c) {
+  return a == c ? 7 : 4;
+}
+
+long primitiveSemantics_compare_reg_return_imm_imm_03(long a, long b, long c) {
+  return a != c ? 7 : 4;
+}
+
+long primitiveSemantics_compare_reg_return_imm_imm_04(long a, long b, long c) {
+  if (a == c)
+    b = 7;
+  else
+    b = 4;
+  return b;
+}
+
+long primitiveSemantics_compare_reg_return_imm_imm_05(long a, long b, long c) {
+  if (!(a == c))
+    b = 7;
+  else
+    b = 4;
+  return b;
+}
+
+int primitiveSemantics_compare_reg_return_imm_imm_06(int a, int b, int c) {
+  return a == c ? 7 : 4;
+}
+
+int primitiveSemantics_compare_reg_return_imm_imm_07(int a, int b, int c) {
+  return a != c ? 7 : 4;
+}
+
+int primitiveSemantics_compare_reg_return_imm_imm_08(int a, int b, int c) {
+  return a == c ? 7 : 4;
+}
+
+int primitiveSemantics_compare_reg_return_imm_imm_09(int a, int b, int c) {
+  return a != c ? 7 : 4;
+}
+
+int primitiveSemantics_compare_reg_return_imm_imm_10(int a, int b, int c) {
+  if ((a == c))
+    b = 7;
+  else
+    b = 4;
+  return b;
+}
+
+int primitiveSemantics_compare_reg_return_imm_imm_11(int a, int b, int c) {
+  if (!(a == c))
+    b = 7;
+  else
+    b = 4;
+  return b;
+}
+
+/* { dg-final { scan-assembler-times {\mczero.eqz\M} 6 } } */
+/* { dg-final { scan-assembler-times {\mczero.nez\M} 6 } } */
+/* { dg-final { scan-assembler-not {\mbeq\M} } } */
+/* { dg-final { scan-assembler-not {\mbne\M} } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_reg.c b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_reg.c
new file mode 100644
index 00000000000..5199ba71ef4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_reg.c
@@ -0,0 +1,65 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f" { target { rv32 } } } */
+/* { dg-skip-if "" { *-*-* } {"-O0" "-Og" "-Os" "-Oz"} } */
+
+long primitiveSemantics_compare_reg_return_imm_reg_00(long a, long b, long c) {
+  return a == c ? 10 : b;
+}
+
+long primitiveSemantics_compare_reg_return_imm_reg_01(long a, long b, long c) {
+  return a != c ? 10 : b;
+}
+
+long primitiveSemantics_compare_reg_return_imm_reg_02(long a, long b, long c) {
+  return a == c ? b : 10;
+}
+
+long primitiveSemantics_compare_reg_return_imm_reg_03(long a, long b, long c) {
+  return a != c ? b : 10;
+}
+
+long primitiveSemantics_compare_reg_return_imm_reg_04(long a, long b, long c) {
+  if (a == c)
+    b = 10;
+  return b;
+}
+
+long primitiveSemantics_compare_reg_return_imm_reg_05(long a, long b, long c) {
+  if (!(a == c))
+    b = 10;
+  return b;
+}
+
+int primitiveSemantics_compare_reg_return_imm_reg_06(int a, int b, int c) {
+  return a == c ? 10 : b;
+}
+
+int primitiveSemantics_compare_reg_return_imm_reg_07(int a, int b, int c) {
+  return a != c ? 10 : b;
+}
+
+int primitiveSemantics_compare_reg_return_imm_reg_08(int a, int b, int c) {
+  return a == c ? b : 10;
+}
+
+int primitiveSemantics_compare_reg_return_imm_reg_09(int a, int b, int c) {
+  return a != c ? b : 10;
+}
+
+int primitiveSemantics_compare_reg_return_imm_reg_10(int a, int b, int c) {
+  if ((a == c))
+    b = 10;
+  return b;
+}
+
+int primitiveSemantics_compare_reg_return_imm_reg_11(int a, int b, int c) {
+  if (!(a == c))
+    b = 10;
+  return b;
+}
+
+/* { dg-final { scan-assembler-times {\mczero.eqz\M} 6 } } */
+/* { dg-final { scan-assembler-times {\mczero.nez\M} 6 } } */
+/* { dg-final { scan-assembler-not {\mbeq\M} } } */
+/* { dg-final { scan-assembler-not {\mbne\M} } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_reg_reg.c b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_reg_reg.c
new file mode 100644
index 00000000000..eecb95688f4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_reg_reg.c
@@ -0,0 +1,77 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f" { target { rv32 } } } */
+/* { dg-skip-if "" { *-*-* } {"-O0" "-Og" "-Os" "-Oz"} } */
+
+long primitiveSemantics_compare_reg_return_reg_reg_00(long a, long b, long c,
+                                                      long d) {
+  return a == c ? d : b;
+}
+
+long primitiveSemantics_compare_reg_return_reg_reg_01(long a, long b, long c,
+                                                      long d) {
+  return a != c ? d : b;
+}
+
+long primitiveSemantics_compare_reg_return_reg_reg_02(long a, long b, long c,
+                                                      long d) {
+  return a == c ? b : d;
+}
+
+long primitiveSemantics_compare_reg_return_reg_reg_03(long a, long b, long c,
+                                                      long d) {
+  return a != c ? b : d;
+}
+
+long primitiveSemantics_compare_reg_return_reg_reg_04(long a, long b, long c,
+                                                      long d) {
+  if (a == c)
+    b = d;
+  return b;
+}
+
+long primitiveSemantics_compare_reg_return_reg_reg_05(long a, long b, long c,
+                                                      long d) {
+  if (!(a == c))
+    b = d;
+  return b;
+}
+
+int primitiveSemantics_compare_reg_return_reg_reg_06(int a, int b, int c,
+                                                     int d) {
+  return a == c ? d : b;
+}
+
+int primitiveSemantics_compare_reg_return_reg_reg_07(int a, int b, int c,
+                                                     int d) {
+  return a != c ? d : b;
+}
+
+int primitiveSemantics_compare_reg_return_reg_reg_08(int a, int b, int c,
+                                                     int d) {
+  return a == c ? b : d;
+}
+
+int primitiveSemantics_compare_reg_return_reg_reg_09(int a, int b, int c,
+                                                     int d) {
+  return a != c ? b : d;
+}
+
+int primitiveSemantics_compare_reg_return_reg_reg_10(int a, int b, int c,
+                                                     int d) {
+  if ((a == c))
+    b = d;
+  return b;
+}
+
+int primitiveSemantics_compare_reg_return_reg_reg_11(int a, int b, int c,
+                                                     int d) {
+  if (!(a == c))
+    b = d;
+  return b;
+}
+
+/* { dg-final { scan-assembler-times {\mczero.eqz\M} 12 } } */
+/* { dg-final { scan-assembler-times {\mczero.nez\M} 12 } } */
+/* { dg-final { scan-assembler-not {\mbeq\M} } } */
+/* { dg-final { scan-assembler-not {\mbne\M} } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c
index 0764d2919d4..e3ccb17032e 100644
--- a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c
+++ b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c
@@ -61,5 +61,5 @@ int primitiveSemantics_return_0_imm_11(int a, int b) {
 
 /* { dg-final { scan-assembler-times {\mczero\.eqz\M} 6 } } */
 /* { dg-final { scan-assembler-times {\mczero\.nez\M} 6 } } */
-/* { dg-final { scan-assembler-not {\mbeq} } } */
-/* { dg-final { scan-assembler-not {\mbne} } } */
+/* { dg-final { scan-assembler-not {\mbeq\M} } } */
+/* { dg-final { scan-assembler-not {\mbne\M} } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c
index 2ff5033bb04..62f9fb29169 100644
--- a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c
+++ b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c
@@ -69,5 +69,5 @@ int primitiveSemantics_return_imm_imm_11(int a, int b) {
 
 /* { dg-final { scan-assembler-times {\mczero\.eqz\M} 6 } } */
 /* { dg-final { scan-assembler-times {\mczero\.nez\M} 6 } } */
-/* { dg-final { scan-assembler-not {\mbeq} } } */
-/* { dg-final { scan-assembler-not {\mbne} } } */
+/* { dg-final { scan-assembler-not {\mbeq\M} } } */
+/* { dg-final { scan-assembler-not {\mbne\M} } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c
index 93844d166c3..0866f86e6ce 100644
--- a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c
+++ b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c
@@ -61,5 +61,5 @@ int primitiveSemantics_return_imm_reg_11(int a, int b) {
 
 /* { dg-final { scan-assembler-times {\mczero\.eqz\M} 6 } } */
 /* { dg-final { scan-assembler-times {\mczero\.nez\M} 6 } } */
-/* { dg-final { scan-assembler-not {\mbeq} } } */
-/* { dg-final { scan-assembler-not {\mbne} } } */
+/* { dg-final { scan-assembler-not {\mbeq\M} } } */
+/* { dg-final { scan-assembler-not {\mbne\M} } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c
index 619ad8ecf7d..eb1764a27ba 100644
--- a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c
+++ b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c
@@ -61,5 +61,5 @@ int primitiveSemantics_return_reg_reg_11(int a, int b, int c) {
 
 /* { dg-final { scan-assembler-times {\mczero\.eqz\M} 12 } } */
 /* { dg-final { scan-assembler-times {\mczero\.nez\M} 12 } } */
-/* { dg-final { scan-assembler-not {\mbeq} } } */
-/* { dg-final { scan-assembler-not {\mbne} } } */
+/* { dg-final { scan-assembler-not {\mbeq\M} } } */
+/* { dg-final { scan-assembler-not {\mbne\M} } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-xor-01.c b/gcc/testsuite/gcc.target/riscv/zicond-xor-01.c
index 8362ffaf5ab..20079fd4351 100644
--- a/gcc/testsuite/gcc.target/riscv/zicond-xor-01.c
+++ b/gcc/testsuite/gcc.target/riscv/zicond-xor-01.c
@@ -10,5 +10,5 @@ long xor1(long crc, long poly)
   return crc;
 }
 
-/* { dg-final { scan-assembler-times "czero.eqz\t" 1 } } */
+/* { dg-final { scan-assembler-times {\mczero.eqz\M} 1 } } */
 /* { dg-final { scan-assembler-times "xor\t" 1 } } */

Re: [committed] RISC-V: Fix INSN costing and more zicond tests

[Hans-Peter Nilsson]

> Date: Fri, 29 Sep 2023 16:37:21 -0600
> From: Jeff Law <jeffreyalaw@gmail.com>

> So this ends up looking a lot like the bits that I had to revert several 
> weeks ago :-)
> 
> The core issue we have is given an INSN the generic code will cost the 
> SET_SRC and SET_DEST and sum them.  But that's far from ideal on a RISC 
> target.
> 
> For a register destination, the cost can be determined be looking at 
> just the SET_SRC.  Which is precisely what this patch does.  When the 
> outer code is an INSN and we're presented with a SET we take one of two 
> paths.
> 
> If the destination is a register, then we recurse just on the SET_SRC 
> and we're done.  Otherwise we fall back to the existing code which sums 
> the cost of the SET_SRC and SET_DEST.

Ackchyually...  that "otherwise" happens for calls to
set_rtx_cost (et al), but not calls to insn_cost.

IOW, with that patch, it seems you're mimicking insn_cost
behavior also for set_rtx_cost (et al).  You're likely aware
of this, but when seeing these target cost functions tweaked
for reasons that appear somewhat empirical, I felt compelled
to point out the related rabbit-hole.

While I'm ranting, these slightly different cost api:s,
somewhat arbitrarily, (or empirically) picked by callers, is
a problem by itself.  Not to mention that the default use of
set_rtx_cost means you get hit by another problem; the
default cost of 0 for registers is also a magic number to
pattern_cost to set the cost to INSN_COSTS (1).

The default insn_cost implementation, which RISC-V uses as
opposed to implementing the TARGET_INSN_COST hook, only
looks at the SET_SRC for calls to insn_cost for single-sets.
See pattern_cost.  I believe that's a bug.  Fixing that was
attempted in 2016 (by Bernd S.), a patch which was later
reverted: cf. commits r7-4866-g334442f282a9d6 and
r7-4930-g03612f25277590.  Hence rabbit-hole.  (And no,
implementing TARGET_INSN_COST doesn't automatically fix
things.  Too much of the gcc middle-end appears tuned to the
default behavior.)

Sorry for the rant; have a nice day and a better week-end.

>  That fallback path isn't great 
> and probably could be further improved (just costing SET_DEST in that 
> case is probably quite reasonable).
> 
> The difference between this version and the bits that slipped through by 
> accident several weeks ago is that old version mis-used the API due to a 
> thinko on my part.
> 
> This tightens up various zicond tests to avoid undesirable matching.
> 
> This has been tested on rv64gc -- the only difference it makes on the 
> testsuite is the new tests (included in this patch) flip from failing to 
> passing.
> 
> Pushed to the trunk.
> 
> Jeff

brgds, H-P

Re: [committed] RISC-V: Fix INSN costing and more zicond tests

[Maciej W. Rozycki]

On Fri, 29 Sep 2023, Jeff Law wrote:

> So this ends up looking a lot like the bits that I had to revert several weeks
> ago :-)
> 
> The core issue we have is given an INSN the generic code will cost the SET_SRC
> and SET_DEST and sum them.  But that's far from ideal on a RISC target.
> 
> For a register destination, the cost can be determined be looking at just the
> SET_SRC.  Which is precisely what this patch does.  When the outer code is an
> INSN and we're presented with a SET we take one of two paths.
> 
> If the destination is a register, then we recurse just on the SET_SRC and
> we're done.  Otherwise we fall back to the existing code which sums the cost
> of the SET_SRC and SET_DEST.  That fallback path isn't great and probably
> could be further improved (just costing SET_DEST in that case is probably
> quite reasonable).

 So this actually breaks insn costing for if-conversion, causing all 
conditional-move expansions to count as 1 insn regardless of how many 
there actually are.  This can be easily verified by using various 
`-mbranch-cost=' settings.

 Before your change you had to set the branch cost to higher than or equal
to the replacement insn count for if-conversion to trigger.  Of course 
tuning microarchitectures will have preset this hopefully correctly for 
their needs, so normally you don't need to resort to `-mbranch-cost='.  
With this change in place only setting `-mbranch-cost=1' will prevent 
if-conversion from triggering, which is taking the situation back to GCC 
13 days, where `movMODEcc' patterns were always cost at 1.

 In preparation for an upcoming set of changes I have written numerous 
testsuite cases to verify this insn costing to work correctly and now that 
I have rebased for the submission all indicate the costing went wrong and 
`movMODEcc' sequences of up to 6 insns are all now cost at 1 total.  I was 
going to post the patch series Fri-Mon, but this seems like a showstopper 
to me, because if-conversion now triggers even when the conditional-move 
(or for that matter conditional-add, as I have it handled too) sequence is 
more expensive than a branched one.

 E.g. the NE operation costs 4 instructions for Zicond:

	sub	a1,a0,a1
	czero.eqz	a2,a2,a1
	czero.nez	a1,a3,a1
	or	a0,a2,a1
	ret

while the branched equivalent costs (branch + 1) instructions:

	beq	a0,a1,.L3
	mv	a0,a2
	ret
.L3:
	mv	a0,a3
	ret

so I'd expect if-conversion only to trigger at `-mbranch-cost=3' or higher 
(just as my test cases verify), but now it triggers at `-mbranch-cost=2' 
already.

 Can we have the insn costing reverted to correct calculation?

  Maciej

Re: [committed] RISC-V: Fix INSN costing and more zicond tests

[Jeff Law]

On 11/9/23 07:33, Maciej W. Rozycki wrote:
> On Fri, 29 Sep 2023, Jeff Law wrote:
> 
>> So this ends up looking a lot like the bits that I had to revert several weeks
>> ago :-)
>>
>> The core issue we have is given an INSN the generic code will cost the SET_SRC
>> and SET_DEST and sum them.  But that's far from ideal on a RISC target.
>>
>> For a register destination, the cost can be determined be looking at just the
>> SET_SRC.  Which is precisely what this patch does.  When the outer code is an
>> INSN and we're presented with a SET we take one of two paths.
>>
>> If the destination is a register, then we recurse just on the SET_SRC and
>> we're done.  Otherwise we fall back to the existing code which sums the cost
>> of the SET_SRC and SET_DEST.  That fallback path isn't great and probably
>> could be further improved (just costing SET_DEST in that case is probably
>> quite reasonable).
> 
>   So this actually breaks insn costing for if-conversion, causing all
> conditional-move expansions to count as 1 insn regardless of how many
> there actually are.  This can be easily verified by using various
> `-mbranch-cost=' settings.
> 
>   Before your change you had to set the branch cost to higher than or equal
> to the replacement insn count for if-conversion to trigger.  Of course
> tuning microarchitectures will have preset this hopefully correctly for
> their needs, so normally you don't need to resort to `-mbranch-cost='.
> With this change in place only setting `-mbranch-cost=1' will prevent
> if-conversion from triggering, which is taking the situation back to GCC
> 13 days, where `movMODEcc' patterns were always cost at 1.
> 
>   In preparation for an upcoming set of changes I have written numerous
> testsuite cases to verify this insn costing to work correctly and now that
> I have rebased for the submission all indicate the costing went wrong and
> `movMODEcc' sequences of up to 6 insns are all now cost at 1 total.  I was
> going to post the patch series Fri-Mon, but this seems like a showstopper
> to me, because if-conversion now triggers even when the conditional-move
> (or for that matter conditional-add, as I have it handled too) sequence is
> more expensive than a branched one.
> 
>   E.g. the NE operation costs 4 instructions for Zicond:
> 
> 	sub	a1,a0,a1
> 	czero.eqz	a2,a2,a1
> 	czero.nez	a1,a3,a1
> 	or	a0,a2,a1
> 	ret
> 
> while the branched equivalent costs (branch + 1) instructions:
> 
> 	beq	a0,a1,.L3
> 	mv	a0,a2
> 	ret
> .L3:
> 	mv	a0,a3
> 	ret
> 
> so I'd expect if-conversion only to trigger at `-mbranch-cost=3' or higher
> (just as my test cases verify), but now it triggers at `-mbranch-cost=2'
> already.
> 
>   Can we have the insn costing reverted to correct calculation?
What needs to happen is that code needs to be extended, not reverted. 
Many codes have to be synthesized based on the condition and the 
true/false arms.  That's not currently accounted for.


jeff

Re: [committed] RISC-V: Fix INSN costing and more zicond tests

[Maciej W. Rozycki]

On Thu, 9 Nov 2023, Jeff Law wrote:

> >   Can we have the insn costing reverted to correct calculation?
> What needs to happen is that code needs to be extended, not reverted. Many
> codes have to be synthesized based on the condition and the true/false arms.
> That's not currently accounted for.

 How is maintaining zillions of variants of insn counts by hand (IIUC what 
you mean) going to be more efficient (or even practical maintenance-wise) 
than what the middle end did automagically?  What exactly was wrong with 
the previous approach, and then why didn't your change include a proof of 
correctness in the form of testsuite cases verifying branch vs conditional 
move costing stays the same (or gets corrected if applicable) across your 
change?

 I guess I'll post my patch series regardless on the presumption that 
correct insn counting will have been reinstated for GCC 14 one way or 
another (i.e. by reverting commit 44efc743acc0 locally in my tree and then 
getting clean test results across the patch series) and we can take it 
from there.  Also to make sure we're on the same page.

 I do hope it will be considered worthwhile despite this issue making it 
not ready for testsuite verification, as not only it adds new features, 
but it fixes numerous existing problems, plain bugs, and deficiencies as 
well which we currently have in conditional move handling.  But it relies 
on correct costing for verification, which I couldn't have expected that 
will get broken again (regardless of your clearly good intentions).  And 
I'd rather we had these test cases or otherwise costing regressions are 
easily missed (as indicated here).

  Maciej

Re: [committed] RISC-V: Fix INSN costing and more zicond tests

[Jeff Law]

On 11/9/23 07:33, Maciej W. Rozycki wrote:
> On Fri, 29 Sep 2023, Jeff Law wrote:
> 
>> So this ends up looking a lot like the bits that I had to revert several weeks
>> ago :-)
>>
>> The core issue we have is given an INSN the generic code will cost the SET_SRC
>> and SET_DEST and sum them.  But that's far from ideal on a RISC target.
>>
>> For a register destination, the cost can be determined be looking at just the
>> SET_SRC.  Which is precisely what this patch does.  When the outer code is an
>> INSN and we're presented with a SET we take one of two paths.
>>
>> If the destination is a register, then we recurse just on the SET_SRC and
>> we're done.  Otherwise we fall back to the existing code which sums the cost
>> of the SET_SRC and SET_DEST.  That fallback path isn't great and probably
>> could be further improved (just costing SET_DEST in that case is probably
>> quite reasonable).
> 
>   So this actually breaks insn costing for if-conversion, causing all
> conditional-move expansions to count as 1 insn regardless of how many
> there actually are.  This can be easily verified by using various
> `-mbranch-cost=' settings.
> 
>   Before your change you had to set the branch cost to higher than or equal
> to the replacement insn count for if-conversion to trigger.  Of course
> tuning microarchitectures will have preset this hopefully correctly for
> their needs, so normally you don't need to resort to `-mbranch-cost='.
> With this change in place only setting `-mbranch-cost=1' will prevent
> if-conversion from triggering, which is taking the situation back to GCC
> 13 days, where `movMODEcc' patterns were always cost at 1.
> 
>   In preparation for an upcoming set of changes I have written numerous
> testsuite cases to verify this insn costing to work correctly and now that
> I have rebased for the submission all indicate the costing went wrong and
> `movMODEcc' sequences of up to 6 insns are all now cost at 1 total.  I was
> going to post the patch series Fri-Mon, but this seems like a showstopper
> to me, because if-conversion now triggers even when the conditional-move
> (or for that matter conditional-add, as I have it handled too) sequence is
> more expensive than a branched one.
> 
>   E.g. the NE operation costs 4 instructions for Zicond:
> 
> 	sub	a1,a0,a1
> 	czero.eqz	a2,a2,a1
> 	czero.nez	a1,a3,a1
> 	or	a0,a2,a1
> 	ret
> 
> while the branched equivalent costs (branch + 1) instructions:
> 
> 	beq	a0,a1,.L3
> 	mv	a0,a2
> 	ret
> .L3:
> 	mv	a0,a3
> 	ret
> 
> so I'd expect if-conversion only to trigger at `-mbranch-cost=3' or higher
> (just as my test cases verify), but now it triggers at `-mbranch-cost=2'
> already.
> 
>   Can we have the insn costing reverted to correct calculation?
FYI, I've opened a bug for this issue so it doesn't get lost.  I don't 
think the extensions are terribly hard.  It's really a matter of 
deciding if we can re-use any of the logic from the expander or if we 
just mirror its logic and keep the expander and costing in sync.

Jeff
> 
>    Maciej