| Message ID | 20260902104819.35962-1-jinma@linux.alibaba.com |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: Fix overlapping vd/vs2 allocation for vector crypto [PR126196] | expand |
On 9/2/26 4:48 AM, Jin Ma wrote: > The RISC-V Vector Crypto specification reserves encodings in which > "the vd register group overlaps with the vs2 register group" for > vsm3me.vv and vsm3c.vi, and in which "the vd register group overlaps > with either vs1 or vs2" for vsha2ms.vv, vsha2ch.vv and vsha2cl.vv. > So mark the destination of the affected define_insn patterns as > early-clobber. > > PR target/126196 > > gcc/ChangeLog: > > * config/riscv/vector-crypto.md (vv_ins1_con): New int attribute. > (vi_ins1_con): Likewise. > (@pred_v<vv_ins1_name><mode>): Mark the destination of > vsha2ms/vsha2ch/vsha2cl as early-clobber. > (@pred_vi<vi_ins1_name><mode>_nomaskedoff_scalar): Likewise for > vsm3c. > (@pred_vsm3me<mode>): Mark the destination as early-clobber. > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/pr126196-1.c: New test. > * gcc.target/riscv/pr126196-2.c: New test. > * gcc.target/riscv/pr126196-3.c: New test. Don't we have the same problem with vaes*.vs and vsm4r.vs? Presumably there's no value in trying to use the work from Robin & Pan which allows for better modeling of when we can and can not have overlapping register/register groups in things like widening vector patterns? Jeff
> On 9/2/26 4:48 AM, Jin Ma wrote: > > The RISC-V Vector Crypto specification reserves encodings in which > > "the vd register group overlaps with the vs2 register group" for > > vsm3me.vv and vsm3c.vi, and in which "the vd register group overlaps > > with either vs1 or vs2" for vsha2ms.vv, vsha2ch.vv and vsha2cl.vv. > > So mark the destination of the affected define_insn patterns as > > early-clobber. > > > > PR target/126196 > > > > gcc/ChangeLog: > > > > * config/riscv/vector-crypto.md (vv_ins1_con): New int attribute. > > (vi_ins1_con): Likewise. > > (@pred_v<vv_ins1_name><mode>): Mark the destination of > > vsha2ms/vsha2ch/vsha2cl as early-clobber. > > (@pred_vi<vi_ins1_name><mode>_nomaskedoff_scalar): Likewise for > > vsm3c. > > (@pred_vsm3me<mode>): Mark the destination as early-clobber. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.target/riscv/pr126196-1.c: New test. > > * gcc.target/riscv/pr126196-2.c: New test. > > * gcc.target/riscv/pr126196-3.c: New test. > Don't we have the same problem with vaes*.vs and vsm4r.vs? Yes. I went back and double-checked, and the same problem does exist. The "Vector/Scalar constraints" subsection (under Instruction Constraints) of the specification explicitly lists: For implementations with VLEN >= 128, vs2 refers to a single register. Thus, the vd register group must not overlap the vs2 register. | Instruction | Register | Cannot Overlap | |---------------|-----------|----------------| | vaes*.vs | vs2 | vd | | vsm4r.vs | vs2 | vd | | vsha2c* | vs1, vs2 | vd | | vsha2ms | vs1, vs2 | vd | | vsm3me | vs2 | vd | | vsm3c | vs2 | vd | So the vd/vs2 overlap of vaes*.vs (including vaesz.vs) and vsm4r.vs is also a reserved encoding, and the "Reserved Encodings" paragraph of each instruction states this individually, e.g. for vsm4r: * Only for the insn:*.vs[] form: the vd register group overlaps the vs2 register I verified all six same-LMUL .vs intrinsic forms on trunk; each of them generates a reserved encoding, e.g.: vuint32m1_t f (vuint32m1_t a, size_t vl) { return __riscv_vaesef_vs_u32m1_u32m1 (a, a, vl); } compiles to "vaesef.vs v8,v8" (likewise for vaesem/vaesdf/vaesdm/ vaesz/vsm4r). This is a pre-existing and separate issue -- this patch does not touch those patterns. I'll send a follow-up patch to fix it. > Presumably there's no value in trying to use the work from Robin & Pan > which allows for better modeling of when we can and can not have > overlapping register/register groups in things like widening vector > patterns? Right, there is no value here. Robin & Pan's work (the dependent filter constraint, e.g. Wtt / riscv_widen_overlap_ok, and the vw* series relaxing early-clobber) goes in the opposite direction: it removes conservative early-clobbers where the specification allows overlap, in order to reduce register pressure, which is a performance concern. What we need here is to forbid overlap entirely, which is a correctness concern, and a plain & is exactly the right tool. The only spot where finer modeling could theoretically pay off is vd/vs1 overlap for vsm3me.vv (the specification only forbids overlap with vs2, so & is conservative there); that could be relaxed with a dependent filter in the future if performance warrants it, but it is outside the scope of this fix. Also, the root cause of the vaes*.vs issue is pattern shadowing rather than register-allocation modeling, so that machinery would not help here either. That said, I'm not sure my understanding is correct -- any comments? Thanks, Jin Ma
On 9/2/26 9:40 AM, Jin Ma wrote: >> > Yes. I went back and double-checked, and the same problem does > exist. The "Vector/Scalar constraints" subsection (under > Instruction Constraints) of the specification explicitly lists: > > For implementations with VLEN >= 128, vs2 refers to a single > register. Thus, the vd register group must not overlap the vs2 > register. > > | Instruction | Register | Cannot Overlap | > |---------------|-----------|----------------| > | vaes*.vs | vs2 | vd | > | vsm4r.vs | vs2 | vd | > | vsha2c* | vs1, vs2 | vd | > | vsha2ms | vs1, vs2 | vd | > | vsm3me | vs2 | vd | > | vsm3c | vs2 | vd | > > So the vd/vs2 overlap of vaes*.vs (including vaesz.vs) and vsm4r.vs is > also a reserved encoding, and the "Reserved Encodings" paragraph of > each instruction states this individually, e.g. for vsm4r: > > * Only for the insn:*.vs[] form: the vd register group overlaps > the vs2 register > > I verified all six same-LMUL .vs intrinsic forms on trunk; each of > them generates a reserved encoding, e.g.: > > vuint32m1_t f (vuint32m1_t a, size_t vl) > { return __riscv_vaesef_vs_u32m1_u32m1 (a, a, vl); } > > compiles to "vaesef.vs v8,v8" (likewise for vaesem/vaesdf/vaesdm/ > vaesz/vsm4r). > > This is a pre-existing and separate issue -- this patch does not > touch those patterns. I'll send a follow-up patch to fix it. Sounds good. > >> Presumably there's no value in trying to use the work from Robin & Pan >> which allows for better modeling of when we can and can not have >> overlapping register/register groups in things like widening vector >> patterns? > Right, there is no value here. Robin & Pan's work (the dependent > filter constraint, e.g. Wtt / riscv_widen_overlap_ok, and the vw* > series relaxing early-clobber) goes in the opposite direction: it > removes conservative early-clobbers where the specification allows > overlap, in order to reduce register pressure, which is a performance > concern. What we need here is to forbid overlap entirely, which is a > correctness concern, and a plain & is exactly the right tool. The > only spot where finer modeling could theoretically pay off is > vd/vs1 overlap for vsm3me.vv (the specification only forbids overlap > with vs2, so & is conservative there); that could be relaxed with a > dependent filter in the future if performance warrants it, but it is > outside the scope of this fix. Also, the root cause of the vaes*.vs > issue is pattern shadowing rather than register-allocation modeling, > so that machinery would not help here either. > > That said, I'm not sure my understanding is correct -- any comments? Understood. So for the vaes* and vsm4r, if you're using the same basic approach as you did for the other vector-crypto, then consider the V2 as pre-approved for the trunk. Thanks, Jeff
diff --git a/gcc/config/riscv/vector-crypto.md b/gcc/config/riscv/vector-crypto.md index b12a8453eb1..25cc6ef9e44 100644 --- a/gcc/config/riscv/vector-crypto.md +++ b/gcc/config/riscv/vector-crypto.md @@ -68,10 +68,19 @@ (define_int_attr vv_ins1_name [(UNSPEC_VGHSH "ghsh") (UNSPEC_VSHA2MS "sha2ms") (UNSPEC_VSHA2CH "sha2ch") (UNSPEC_VSHA2CL "sha2cl")]) +;; vd overlapping vs1 or vs2 is reserved for vsha2*, but not for vghsh. +(define_int_attr vv_ins1_con + [(UNSPEC_VGHSH "=vr") (UNSPEC_VSHA2MS "=&vr") + (UNSPEC_VSHA2CH "=&vr") (UNSPEC_VSHA2CL "=&vr")]) + (define_int_attr vi_ins_name [(UNSPEC_VAESKF1 "aeskf1") (UNSPEC_VSM4K "sm4k")]) (define_int_attr vi_ins1_name [(UNSPEC_VAESKF2 "aeskf2") (UNSPEC_VSM3C "sm3c")]) +;; vd overlapping vs2 is reserved for vsm3c, but not for vaeskf2. +(define_int_attr vi_ins1_con + [(UNSPEC_VAESKF2 "=vr") (UNSPEC_VSM3C "=&vr")]) + (define_int_attr ins_type [(UNSPEC_VGMUL "vv") (UNSPEC_VAESEFVV "vv") (UNSPEC_VAESEMVV "vv") (UNSPEC_VAESDFVV "vv") (UNSPEC_VAESDMVV "vv") (UNSPEC_VAESEFVS "vs") @@ -464,7 +473,7 @@ ;; zvknh[ab] and zvkg instructions patterns. ;; vsha2ms.vv vsha2ch.vv vsha2cl.vv vghsh.vv (define_insn "@pred_v<vv_ins1_name><mode>" - [(set (match_operand:VQEXTI 0 "register_operand" "=vr") + [(set (match_operand:VQEXTI 0 "register_operand" "<vv_ins1_con>") (if_then_else:VQEXTI (unspec:<VM> [(match_operand 4 "vector_length_operand" "rK") @@ -615,7 +624,7 @@ ;; vaeskf2.vi vsm3c.vi (define_insn "@pred_vi<vi_ins1_name><mode>_nomaskedoff_scalar" - [(set (match_operand:V_VLSI_S 0 "register_operand" "=vr") + [(set (match_operand:V_VLSI_S 0 "register_operand" "<vi_ins1_con>") (if_then_else:V_VLSI_S (unspec:<VM> [(match_operand 4 "vector_length_operand" "rK") @@ -636,7 +645,7 @@ ;; zvksh instructions patterns. ;; vsm3me.vv (define_insn "@pred_vsm3me<mode>" - [(set (match_operand:V_VLSI_S 0 "register_operand" "=vr, vr") + [(set (match_operand:V_VLSI_S 0 "register_operand" "=&vr, &vr") (if_then_else:V_VLSI_S (unspec:<VM> [(match_operand 4 "vector_length_operand" " rK, rK") diff --git a/gcc/testsuite/gcc.target/riscv/pr126196-1.c b/gcc/testsuite/gcc.target/riscv/pr126196-1.c new file mode 100644 index 00000000000..f94a3f3b38d --- /dev/null +++ b/gcc/testsuite/gcc.target/riscv/pr126196-1.c @@ -0,0 +1,29 @@ +/* { dg-do compile } */ +/* { dg-options "-march=rv64gcv_zvksh -mabi=lp64d -O2" { target { rv64 } } } */ +/* { dg-options "-march=rv32gcv_zvksh -mabi=ilp32d -O2" { target { rv32 } } } */ +/* { dg-skip-if "" { *-*-* } { "-O0" "-O1" "-Os" "-Oz" "-Og" } } */ + +#include <riscv_vector.h> + +vuint32m1_t +f (vuint32m1_t vs2, vuint32m1_t vs1, size_t vl) +{ + return __riscv_vsm3me_vv_u32m1 (vs2, + __riscv_vsm3me_vv_u32m1 (vs2, vs1, vl), + vl); +} + +vuint32m1_t +g (vuint32m1_t a, vuint32m1_t b, vuint32m1_t c, vuint32m1_t d, + vuint32m1_t e, vuint32m1_t h, vuint32m1_t i, vuint32m1_t j, + size_t vl) +{ + vuint32m1_t r1 = __riscv_vsm3me_vv_u32m1 (a, b, vl); + vuint32m1_t r2 = __riscv_vsm3me_vv_u32m1 (c, d, vl); + vuint32m1_t r3 = __riscv_vsm3me_vv_u32m1 (e, h, vl); + vuint32m1_t r4 = __riscv_vsm3me_vv_u32m1 (i, j, vl); + return __riscv_vxor_vv_u32m1 (__riscv_vxor_vv_u32m1 (r1, r2, vl), + __riscv_vxor_vv_u32m1 (r3, r4, vl), vl); +} + +/* { dg-final { scan-assembler-not {vsm3me\.vv\tv([0-9]+),v\1,} } } */ diff --git a/gcc/testsuite/gcc.target/riscv/pr126196-2.c b/gcc/testsuite/gcc.target/riscv/pr126196-2.c new file mode 100644 index 00000000000..e85b7e362a4 --- /dev/null +++ b/gcc/testsuite/gcc.target/riscv/pr126196-2.c @@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-options "-march=rv64gcv_zvkned_zvksh -mabi=lp64d -O2" { target { rv64 } } } */ +/* { dg-options "-march=rv32gcv_zvkned_zvksh -mabi=ilp32d -O2" { target { rv32 } } } */ +/* { dg-skip-if "" { *-*-* } { "-O0" "-O1" "-Os" "-Oz" "-Og" } } */ + +#include <riscv_vector.h> + +vuint32m1_t +f (vuint32m1_t a, size_t vl) +{ + return __riscv_vsm3c_vi_u32m1 (a, a, 2, vl); +} + +vuint32m1_t +g (vuint32m1_t a, size_t vl) +{ + return __riscv_vaeskf2_vi_u32m1 (a, a, 3, vl); +} + +/* { dg-final { scan-assembler-not {vsm3c\.vi\tv([0-9]+),v\1,} } } */ +/* { dg-final { scan-assembler {vaeskf2\.vi\tv([0-9]+),v\1,} } } */ diff --git a/gcc/testsuite/gcc.target/riscv/pr126196-3.c b/gcc/testsuite/gcc.target/riscv/pr126196-3.c new file mode 100644 index 00000000000..ffa1553d48b --- /dev/null +++ b/gcc/testsuite/gcc.target/riscv/pr126196-3.c @@ -0,0 +1,26 @@ +/* { dg-do compile } */ +/* { dg-options "-march=rv64gcv_zvknhb_zvkg -mabi=lp64d -O2" { target { rv64 } } } */ +/* { dg-options "-march=rv32gcv_zvknhb_zvkg -mabi=ilp32d -O2" { target { rv32 } } } */ +/* { dg-skip-if "" { *-*-* } { "-O0" "-O1" "-Os" "-Oz" "-Og" } } */ + +#include <riscv_vector.h> + +vuint32m1_t +f (vuint32m1_t a, vuint32m1_t b, size_t vl) +{ + vuint32m1_t r = __riscv_vsha2ms_vv_u32m1 (a, a, b, vl); + r = __riscv_vsha2ms_vv_u32m1 (r, b, r, vl); + r = __riscv_vsha2ch_vv_u32m1 (r, r, b, vl); + r = __riscv_vsha2ch_vv_u32m1 (r, b, r, vl); + r = __riscv_vsha2cl_vv_u32m1 (r, r, b, vl); + r = __riscv_vsha2cl_vv_u32m1 (r, b, r, vl); + return __riscv_vghsh_vv_u32m1 (r, r, b, vl); +} + +/* { dg-final { scan-assembler-not {vsha2ms\.vv\tv([0-9]+),v\1,} } } */ +/* { dg-final { scan-assembler-not {vsha2ms\.vv\tv([0-9]+),v[0-9]+,v\1\s} } } */ +/* { dg-final { scan-assembler-not {vsha2ch\.vv\tv([0-9]+),v\1,} } } */ +/* { dg-final { scan-assembler-not {vsha2ch\.vv\tv([0-9]+),v[0-9]+,v\1\s} } } */ +/* { dg-final { scan-assembler-not {vsha2cl\.vv\tv([0-9]+),v\1,} } } */ +/* { dg-final { scan-assembler-not {vsha2cl\.vv\tv([0-9]+),v[0-9]+,v\1\s} } } */ +/* { dg-final { scan-assembler {vghsh\.vv\tv([0-9]+),v\1,} } } */