Re: [PATCH v5] RISC-V: Fix register overlap issue for some xtheadvector instructions
LGTM now, thanks. I find it much more readable that way. Regards Robin
Re: [PATCH v5] RISC-V: Fix register overlap issue for some xtheadvector instructions
Ok from myside. CCing Robin to see whether he has any more concerns. Thanks. juzhe.zh...@rivai.ai From: Jun Sha (Joshua) Date: 2024-01-11 10:39 To: gcc-patches CC: jim.wilson.gcc; palmer; andrew; philipp.tomsich; jeffreyalaw; christoph.muellner; juzhe.zhong; Jun Sha (Joshua); Jin Ma; Xianmiao Qu Subject: [PATCH v5] RISC-V: Fix register overlap issue for some xtheadvector instructions For th.vmadc/th.vmsbc as well as narrowing arithmetic instructions and floating-point compare instructions, an illegal instruction exception will be raised if the destination vector register overlaps a source vector register group. To handle this issue, we add an attribute "spec_restriction" to disable some alternatives for xtheadvector. gcc/ChangeLog: * config/riscv/riscv.md (none,thv,rvv): (no,yes): Add an attribute to disable alternative for xtheadvector or RVV1.0. * config/riscv/vector.md: Disable alternatives that destination register overlaps source register group for xtheadvector. Co-authored-by: Jin Ma Co-authored-by: Xianmiao Qu Co-authored-by: Christoph Müllner --- gcc/config/riscv/riscv.md | 22 +++ gcc/config/riscv/vector.md | 314 + 2 files changed, 202 insertions(+), 134 deletions(-) diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md index 84212430dc0..23fc32d5cb2 100644 --- a/gcc/config/riscv/riscv.md +++ b/gcc/config/riscv/riscv.md @@ -579,6 +579,25 @@ ] (const_string "yes"))) +;; This attribute marks the alternatives not matching the constraints +;; described in spec as disabled. +(define_attr "spec_restriction" "none,thv,rvv" + (const_string "none")) + +(define_attr "spec_restriction_disabled" "no,yes" + (cond [(eq_attr "spec_restriction" "none") + (const_string "no") + + (and (eq_attr "spec_restriction" "thv") + (match_test "TARGET_XTHEADVECTOR")) + (const_string "yes") + + (and (eq_attr "spec_restriction" "rvv") + (match_test "TARGET_VECTOR && !TARGET_XTHEADVECTOR")) + (const_string "yes") + ] + (const_string "no"))) + ;; Attribute to control enable or disable instructions. (define_attr "enabled" "no,yes" (cond [ @@ -590,6 +609,9 @@ (eq_attr "group_overlap_valid" "no") (const_string "no") + +(eq_attr "spec_restriction_disabled" "yes") +(const_string "no") ] (const_string "yes"))) diff --git a/gcc/config/riscv/vector.md b/gcc/config/riscv/vector.md index 3eb6daafbc2..c79416cf0d3 100644 --- a/gcc/config/riscv/vector.md +++ b/gcc/config/riscv/vector.md @@ -3260,7 +3260,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "spec_restriction" "thv,none,none")]) (define_insn "@pred_msbc" [(set (match_operand: 0 "register_operand""=vr, vr, ") @@ -3279,7 +3280,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "spec_restriction" "thv,thv,none")]) (define_insn "@pred_madc_scalar" [(set (match_operand: 0 "register_operand" "=vr, ") @@ -3299,7 +3301,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "spec_restriction" "thv,none")]) (define_insn "@pred_msbc_scalar" [(set (match_operand: 0 "register_operand" "=vr, ") @@ -3319,7 +3322,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "spec_restriction" "thv,none")]) (define_expand "@pred_madc_scalar" [(set (match_operand: 0 "register_operand") @@ -3368,7 +3372,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "spec_restriction" "thv,none")]) (define_insn "*pred_madc_extended_scalar" [(set (match_operand: 0 "register_operand" "=vr, ") @@ -3389,7 +3394,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "spec_restriction" "thv,none")]) (define_expand "@pred_msbc_scalar" [(set (match_operand: 0 "register_operand") @@ -3438,7 +3444,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "spec_restriction" "thv,none")]) (define_insn "*pred_msbc_extended_scalar" [(set (match_operand: 0 "register_operand" "=vr, ") @@ -3459,7 +3466,8 @@ [(set_attr
Re: Re: [PATCH v5] RISC-V: Fix register overlap issue for some xtheadvector instructions
>> For the other insns, I wonder if we could get away with not really
>>disabling the newly added early-clobber alternatives for RVV but
>>just disparaging ("?") them? That way we could re-use "full" for
>>the thv-disabled alternatives and "none" for the newly added ones.
>>("none" will still be misleading then, though :/)
I prefer to disable those early-clobber alternatives added of theadvector for
RVV,
since disparage still make RA possible reaches the early clobber alternatives.
>>If this doesn't work or others feel the separation is not strict
>>enough, I'd prefer a separate attribute rather than overloading
>>group_overlap. Maybe something like "spec_restriction" or similar
>>with two values "rvv" and "thv"?
I like this idea, it makes more sense to me. So I think it's better to add an
attribute to
disable alternative for theadvector or RVV1.0.
juzhe.zh...@rivai.ai
From: Robin Dapp
Date: 2024-01-10 21:36
To: Jun Sha (Joshua); gcc-patches
CC: rdapp.gcc; jim.wilson.gcc; palmer; andrew; philipp.tomsich; jeffreyalaw;
christoph.muellner; juzhe.zhong; Jin Ma; Xianmiao Qu
Subject: Re: [PATCH v5] RISC-V: Fix register overlap issue for some
xtheadvector instructions
Hi Joshua,
> For th.vmadc/th.vmsbc as well as narrowing arithmetic instructions
> and floating-point compare instructions, an illegal instruction
> exception will be raised if the destination vector register overlaps
> a source vector register group.
>
> To handle this issue, we use "group_overlap" and "enabled" attribute
> to disable some alternatives for xtheadvector.
> ;; Widening instructions have group-overlap constraints. Those are only
> ;; valid for certain register-group sizes. This attribute marks the
> ;; alternatives not matching the required register-group size as disabled.
> -(define_attr "group_overlap" "none,W21,W42,W84,W43,W86,W87,W0"
> +(define_attr "group_overlap"
> "none,W21,W42,W84,W43,W86,W87,W0,thv_disabled,rvv_disabled"
>(const_string "none"))
I realize there have been some discussions before but I find the naming
misleading. The group_overlap attribute is supposed to specify whether
groups overlap (and mark the respective alternatives accepting
only this overlap).
Then we check if the groups overlap and disable all non-matching
alternatives. "none" i.e. "no overlap" always matches.
Your first goal seems to be to disable existing non-early-clobber
alternatives for thv. For this, maybe "full", "same" (or "any"?) would
work? Please also add a comment in group_overlap_valid then that we
need not actually check for register equality.
For the other insns, I wonder if we could get away with not really
disabling the newly added early-clobber alternatives for RVV but
just disparaging ("?") them? That way we could re-use "full" for
the thv-disabled alternatives and "none" for the newly added ones.
("none" will still be misleading then, though :/)
If this doesn't work or others feel the separation is not strict
enough, I'd prefer a separate attribute rather than overloading
group_overlap. Maybe something like "spec_restriction" or similar
with two values "rvv" and "thv"?
Regards
Robin
Re: [PATCH v5] RISC-V: Fix register overlap issue for some xtheadvector instructions
Hi Joshua,
> For th.vmadc/th.vmsbc as well as narrowing arithmetic instructions
> and floating-point compare instructions, an illegal instruction
> exception will be raised if the destination vector register overlaps
> a source vector register group.
>
> To handle this issue, we use "group_overlap" and "enabled" attribute
> to disable some alternatives for xtheadvector.
> ;; Widening instructions have group-overlap constraints. Those are only
> ;; valid for certain register-group sizes. This attribute marks the
> ;; alternatives not matching the required register-group size as disabled.
> -(define_attr "group_overlap" "none,W21,W42,W84,W43,W86,W87,W0"
> +(define_attr "group_overlap"
> "none,W21,W42,W84,W43,W86,W87,W0,thv_disabled,rvv_disabled"
>(const_string "none"))
I realize there have been some discussions before but I find the naming
misleading. The group_overlap attribute is supposed to specify whether
groups overlap (and mark the respective alternatives accepting
only this overlap).
Then we check if the groups overlap and disable all non-matching
alternatives. "none" i.e. "no overlap" always matches.
Your first goal seems to be to disable existing non-early-clobber
alternatives for thv. For this, maybe "full", "same" (or "any"?) would
work? Please also add a comment in group_overlap_valid then that we
need not actually check for register equality.
For the other insns, I wonder if we could get away with not really
disabling the newly added early-clobber alternatives for RVV but
just disparaging ("?") them? That way we could re-use "full" for
the thv-disabled alternatives and "none" for the newly added ones.
("none" will still be misleading then, though :/)
If this doesn't work or others feel the separation is not strict
enough, I'd prefer a separate attribute rather than overloading
group_overlap. Maybe something like "spec_restriction" or similar
with two values "rvv" and "thv"?
Regards
Robin
Re: [PATCH v5] RISC-V: Fix register overlap issue for some xtheadvector instructions
LGTM from myside. Give another a few more days that some one want to chime in. juzhe.zh...@rivai.ai From: Jun Sha (Joshua) Date: 2024-01-10 14:51 To: gcc-patches CC: jim.wilson.gcc; palmer; andrew; philipp.tomsich; jeffreyalaw; christoph.muellner; juzhe.zhong; Jun Sha (Joshua); Jin Ma; Xianmiao Qu Subject: [PATCH v5] RISC-V: Fix register overlap issue for some xtheadvector instructions For th.vmadc/th.vmsbc as well as narrowing arithmetic instructions and floating-point compare instructions, an illegal instruction exception will be raised if the destination vector register overlaps a source vector register group. To handle this issue, we use "group_overlap" and "enabled" attribute to disable some alternatives for xtheadvector. gcc/ChangeLog: * config/riscv/riscv.md (none,W21,W42,W84,W43,W86,W87,W0): (none,W21,W42,W84,W43,W86,W87,W0,thv_disabled,rvv_disabled): Add group-overlap constraint for xtheadvector. * config/riscv/vector.md: Disable alternatives that destination register overlaps source register group for xtheadvector. Co-authored-by: Jin Ma Co-authored-by: Xianmiao Qu Co-authored-by: Christoph Müllner --- gcc/config/riscv/riscv.md | 12 +- gcc/config/riscv/vector.md | 314 + 2 files changed, 190 insertions(+), 136 deletions(-) diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md index 84212430dc0..411d1d17391 100644 --- a/gcc/config/riscv/riscv.md +++ b/gcc/config/riscv/riscv.md @@ -537,7 +537,7 @@ ;; Widening instructions have group-overlap constraints. Those are only ;; valid for certain register-group sizes. This attribute marks the ;; alternatives not matching the required register-group size as disabled. -(define_attr "group_overlap" "none,W21,W42,W84,W43,W86,W87,W0" +(define_attr "group_overlap" "none,W21,W42,W84,W43,W86,W87,W0,thv_disabled,rvv_disabled" (const_string "none")) (define_attr "group_overlap_valid" "no,yes" @@ -576,7 +576,15 @@ (and (eq_attr "group_overlap" "W0") (match_test "riscv_get_v_regno_alignment (GET_MODE (operands[0])) > 1")) (const_string "no") -] + + (and (eq_attr "group_overlap" "thv_disabled") + (match_test "TARGET_XTHEADVECTOR")) + (const_string "no") + + (and (eq_attr "group_overlap" "rvv_disabled") + (match_test "TARGET_VECTOR && !TARGET_XTHEADVECTOR")) + (const_string "no") + ] (const_string "yes"))) ;; Attribute to control enable or disable instructions. diff --git a/gcc/config/riscv/vector.md b/gcc/config/riscv/vector.md index 3eb6daafbc2..4748ddd34a2 100644 --- a/gcc/config/riscv/vector.md +++ b/gcc/config/riscv/vector.md @@ -3260,7 +3260,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "group_overlap" "thv_disabled,none,none")]) (define_insn "@pred_msbc" [(set (match_operand: 0 "register_operand""=vr, vr, ") @@ -3279,7 +3280,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "group_overlap" "thv_disabled,thv_disabled,none")]) (define_insn "@pred_madc_scalar" [(set (match_operand: 0 "register_operand" "=vr, ") @@ -3299,7 +3301,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "group_overlap" "thv_disabled,none")]) (define_insn "@pred_msbc_scalar" [(set (match_operand: 0 "register_operand" "=vr, ") @@ -3319,7 +3322,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "group_overlap" "thv_disabled,none")]) (define_expand "@pred_madc_scalar" [(set (match_operand: 0 "register_operand") @@ -3368,7 +3372,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "group_overlap" "thv_disabled,none")]) (define_insn "*pred_madc_extended_scalar" [(set (match_operand: 0 "register_operand" "=vr, ") @@ -3389,7 +3394,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "group_overlap" "thv_disabled,none")]) (define_expand "@pred_msbc_scalar" [(set (match_operand: 0 "register_operand") @@ -3438,7 +3444,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "group_overlap"
Re: [PATCH v5] RISC-V: Fix register overlap issue for some xtheadvector instructions
+ (and (eq_attr "group_overlap" "th") + (match_test "TARGET_XTHEADVECTOR")) + (const_string "no") + + (and (eq_attr "group_overlap" "rvv") + (match_test "TARGET_VECTOR && !TARGET_XTHEADVECTOR")) + (const_string "no") + ] Change it into: + (and (eq_attr "group_overlap" "thv_disabled") + (match_test "TARGET_XTHEADVECTOR")) + (const_string "no") + + (and (eq_attr "group_overlap" "rvv_disabled") + (match_test "TARGET_VECTOR && !TARGET_XTHEADVECTOR")) + (const_string "no") + ] juzhe.zh...@rivai.ai From: Jun Sha (Joshua) Date: 2024-01-10 14:02 To: gcc-patches CC: jim.wilson.gcc; palmer; andrew; philipp.tomsich; jeffreyalaw; christoph.muellner; juzhe.zhong; Jun Sha (Joshua); Jin Ma; Xianmiao Qu Subject: [PATCH v5] RISC-V: Fix register overlap issue for some xtheadvector instructions For th.vmadc/th.vmsbc as well as narrowing arithmetic instructions and floating-point compare instructions, an illegal instruction exception will be raised if the destination vector register overlaps a source vector register group. To handle this issue, we use "group_overlap" and "enabled" attribute to disable some alternatives for xtheadvector. gcc/ChangeLog: * config/riscv/riscv.md (none,W21,W42,W84,W43,W86,W87,W0): (none,W21,W42,W84,W43,W86,W87,W0,th): Add group-overlap constraint for xtheadvector. * config/riscv/vector.md: Disable alternatives that destination register overlaps source register group for xtheadvector. Co-authored-by: Jin Ma Co-authored-by: Xianmiao Qu Co-authored-by: Christoph Müllner --- gcc/config/riscv/riscv.md | 12 +- gcc/config/riscv/vector.md | 314 + 2 files changed, 190 insertions(+), 136 deletions(-) diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md index 84212430dc0..2fe15fd7340 100644 --- a/gcc/config/riscv/riscv.md +++ b/gcc/config/riscv/riscv.md @@ -537,7 +537,7 @@ ;; Widening instructions have group-overlap constraints. Those are only ;; valid for certain register-group sizes. This attribute marks the ;; alternatives not matching the required register-group size as disabled. -(define_attr "group_overlap" "none,W21,W42,W84,W43,W86,W87,W0" +(define_attr "group_overlap" "none,W21,W42,W84,W43,W86,W87,W0,th,rvv" (const_string "none")) (define_attr "group_overlap_valid" "no,yes" @@ -576,7 +576,15 @@ (and (eq_attr "group_overlap" "W0") (match_test "riscv_get_v_regno_alignment (GET_MODE (operands[0])) > 1")) (const_string "no") -] + + (and (eq_attr "group_overlap" "th") + (match_test "TARGET_XTHEADVECTOR")) + (const_string "no") + + (and (eq_attr "group_overlap" "rvv") + (match_test "TARGET_VECTOR && !TARGET_XTHEADVECTOR")) + (const_string "no") + ] (const_string "yes"))) ;; Attribute to control enable or disable instructions. diff --git a/gcc/config/riscv/vector.md b/gcc/config/riscv/vector.md index 3eb6daafbc2..cd83c1f3321 100644 --- a/gcc/config/riscv/vector.md +++ b/gcc/config/riscv/vector.md @@ -3260,7 +3260,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "group_overlap" "th,none,none")]) (define_insn "@pred_msbc" [(set (match_operand: 0 "register_operand""=vr, vr, ") @@ -3279,7 +3280,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "group_overlap" "th,th,none")]) (define_insn "@pred_madc_scalar" [(set (match_operand: 0 "register_operand" "=vr, ") @@ -3299,7 +3301,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "group_overlap" "th,none")]) (define_insn "@pred_msbc_scalar" [(set (match_operand: 0 "register_operand" "=vr, ") @@ -3319,7 +3322,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "group_overlap" "th,none")]) (define_expand "@pred_madc_scalar" [(set (match_operand: 0 "register_operand") @@ -3368,7 +3372,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "group_overlap" "th,none")]) (define_insn "*pred_madc_extended_scalar" [(set (match_operand: 0 "register_operand" "=vr, ") @@ -3389,7 +3394,8 @@ [(set_attr "type" "vicalu") (set_attr "mode" "") (set_attr "vl_op_idx" "4") - (set (attr "avl_type_idx") (const_int 5))]) + (set (attr "avl_type_idx") (const_int 5)) + (set_attr "group_overlap" "th,none")]) (define_expand "@pred_msbc_scalar"
