On 10/2/26 3:20 AM, Michiel Derhaeg wrote:
From: Loeka Rogge<[email protected]>
This removes the guards that disables the clmulr pattern added by
patch r17-4206-ge91f79891fc657: Use clmulr for reversed crc32 on rv32
from being used on rv64.
This changes the calculation of the reflected quotient (on the host) to
fit inside 64 bits. gf2n_poly_long_div_quotient calculates
x^128 / (x^64 + lower order terms) = x^64 + lower order terms.
Meaning the result is missing an implicit 1 in front of the msb.
After reversing, this missing 1 should be after the lsb. This is
re-added with a shift and an or. This causes the msb to fall outside
the result stored in t0.
The value in t0 is only used by the emitted clmul or vclmul
instructions. Clmul only emits the bottom 64 bits, representing x^63 and
lower order terms. This is unaffected by the missing bit that
represents x^64, because it would always fall outside the result.
Regtested on rv32gc, rv32gc_zbc, rv64gc, and rv64gc_zbc.
gcc/ChangeLog:
* config/riscv/bitmanip.md: remove 64 bit guard.
* config/riscv/riscv.cc (riscv_optab_supported_p): remove
64 bit guard.
(expand_reversed_crc_using_clmul): Adapt quotient calculation
for 65 bits
* expr.cc (gf2n_poly_long_div_quotient): remove 64 bit guard
gcc/testsuite/ChangeLog:
* gcc.target/riscv/crc-builtin-zbc64.c: test whether clmulr is
applied for crc64
Signed-off-by: Loeka Rogge<[email protected]>
Thanks for following up on this. Just a few small things.
---
gcc/config/riscv/bitmanip.md | 5 +--
gcc/config/riscv/riscv.cc | 36 ++++++++++++++-----
gcc/expr.cc | 6 ++--
.../gcc.target/riscv/crc-builtin-zbc64.c | 23 +++++++++++-
4 files changed, 54 insertions(+), 16 deletions(-)
diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
index 55bbf304bff..e3c00dd0e42 100644
--- a/gcc/config/riscv/bitmanip.md
+++ b/gcc/config/riscv/bitmanip.md
@@ -1404,10 +1404,11 @@
(E.g. CRC64's quotient may need 65 bits,
we can't keep it in 64 bit variable.)
then use clmul instruction to implement the CRC.
- We can also use clmulr for CRC-32 on RV32, requiring ZBC. */
+ We can also use clmulr for CRC-32 on RV32 and CRC-64 on RV64,
+ requiring ZBC. */
Should the earlier part of the comment indicating we can't handle CRC64
due to the quotient potentially needing 64 bits need updating? THere's
a second copy a couple dozen lines later.
if (((TARGET_ZBKC || TARGET_ZBC || TARGET_ZVBC)
&& <ANYI:MODE>mode < word_mode)
- || (!TARGET_64BIT && TARGET_ZBC && <ANYI:MODE>mode == word_mode))
+ || (TARGET_ZBC && <ANYI:MODE>mode == word_mode))
expand_reversed_crc_using_clmul (<ANYI:MODE>mode, <ANYI1:MODE>mode,
operands);
else
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index ae3fd8e3d72..1085a71ac75 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -13498,7 +13498,7 @@ riscv_optab_supported_p (int op, machine_mode,
machine_mode result_mode,
if (op == crc_rev_optab && opt_type != OPTIMIZE_FOR_SPEED)
return (((TARGET_ZBKC || TARGET_ZBC || TARGET_ZVBC)
&& result_mode < word_mode)
- || (!TARGET_64BIT && TARGET_ZBC && result_mode == word_mode));
+ || (TARGET_ZBC && result_mode == word_mode));
return true;
}
@@ -15942,15 +15942,31 @@ expand_reversed_crc_using_clmul (scalar_mode
crc_mode, scalar_mode data_mode,
gcc_assert (!CONST_INT_P (operands[0]));
gcc_assert (CONST_INT_P (operands[3]));
unsigned short crc_size = GET_MODE_BITSIZE (crc_mode);
- gcc_assert (crc_size <= 32);
+ gcc_assert (crc_size <= 64);
Would this be better checking crc_size <= BITS_PER_WORD? That would
guard against a 64bit crc on rv32. I don't think we can ever get into
this code in that scenario, but if we're going to have an assert, might
as well make it as tight as reasonably possible.
diff --git a/gcc/expr.cc b/gcc/expr.cc
index a9157dc5d41..0dd9d7d8870 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -14563,16 +14563,14 @@ int_expr_size (const_tree exp)
}
/* Return the quotient of polynomial long division of x^2N by POLYNOMIAL
- in GF (2^N).
+ in GF (2^N). The result has degree N, so if N > 63, the result will
+ not fit in a HOST_WIDE_INT. This must be taken into account by the caller.
Author: Richard Sandiford<[email protected]> */
unsigned HOST_WIDE_INT
gf2n_poly_long_div_quotient (unsigned HOST_WIDE_INT polynomial,
unsigned short n)
{
- /* The result has degree N, so needs N + 1 bits. */
- gcc_assert (n < 64);
Would it make sense to test n <= 64 here?
LLM review suggested a CRC64 test run test with a dense polynomial
like 0x42f0e1eba9ea3693ull as well as initial values that have some
bits set in 32..63 range. Not strictly required, but nice to have.
No concerns about the core of the patch. Inclined to ack with the
assert adjustments and comment fixes. Love to have a 64bit run test,
but I won't demand it.
jeff
