* [committed] RISC-V: Fix INSN costing and more zicond tests
@ 2023-09-29 22:37 Jeff Law
2023-10-13 4:02 ` Hans-Peter Nilsson
2023-11-09 14:33 ` Maciej W. Rozycki
0 siblings, 2 replies; 6+ messages in thread
From: Jeff Law @ 2023-09-29 22:37 UTC (permalink / raw)
To: gcc-patches
[-- 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 } } */
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [committed] RISC-V: Fix INSN costing and more zicond tests
2023-09-29 22:37 [committed] RISC-V: Fix INSN costing and more zicond tests Jeff Law
@ 2023-10-13 4:02 ` Hans-Peter Nilsson
2023-11-09 14:33 ` Maciej W. Rozycki
1 sibling, 0 replies; 6+ messages in thread
From: Hans-Peter Nilsson @ 2023-10-13 4:02 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [committed] RISC-V: Fix INSN costing and more zicond tests
2023-09-29 22:37 [committed] RISC-V: Fix INSN costing and more zicond tests Jeff Law
2023-10-13 4:02 ` Hans-Peter Nilsson
@ 2023-11-09 14:33 ` Maciej W. Rozycki
2023-11-09 15:03 ` Jeff Law
2023-11-09 15:47 ` Jeff Law
1 sibling, 2 replies; 6+ messages in thread
From: Maciej W. Rozycki @ 2023-11-09 14:33 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [committed] RISC-V: Fix INSN costing and more zicond tests
2023-11-09 14:33 ` Maciej W. Rozycki
@ 2023-11-09 15:03 ` Jeff Law
2023-11-10 1:32 ` Maciej W. Rozycki
2023-11-09 15:47 ` Jeff Law
1 sibling, 1 reply; 6+ messages in thread
From: Jeff Law @ 2023-11-09 15:03 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [committed] RISC-V: Fix INSN costing and more zicond tests
2023-11-09 15:03 ` Jeff Law
@ 2023-11-10 1:32 ` Maciej W. Rozycki
0 siblings, 0 replies; 6+ messages in thread
From: Maciej W. Rozycki @ 2023-11-10 1:32 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [committed] RISC-V: Fix INSN costing and more zicond tests
2023-11-09 14:33 ` Maciej W. Rozycki
2023-11-09 15:03 ` Jeff Law
@ 2023-11-09 15:47 ` Jeff Law
1 sibling, 0 replies; 6+ messages in thread
From: Jeff Law @ 2023-11-09 15:47 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-11-10 1:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-29 22:37 [committed] RISC-V: Fix INSN costing and more zicond tests Jeff Law
2023-10-13 4:02 ` Hans-Peter Nilsson
2023-11-09 14:33 ` Maciej W. Rozycki
2023-11-09 15:03 ` Jeff Law
2023-11-10 1:32 ` Maciej W. Rozycki
2023-11-09 15:47 ` 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).