| Message ID | 87plwwjx55.fsf@euler.schwinge.ddns.net |
|---|---|
| State | New |
| Headers | show |
| Series | GCN: Conditionalize 'define_expand "reduc_<fexpander>_scal_<mode>"' on '!TARGET_RDNA2_PLUS' [PR113615] (was: [patch] gcn/gcn-valu.md: Disable fold_left_plus for TARGET_RDNA2_PLUS [PR113615]) | expand |
On 16/02/2024 14:34, Thomas Schwinge wrote: > Hi! > > On 2024-01-29T11:34:05+0100, Tobias Burnus <tburnus@baylibre.com> wrote: >> Andrew wrote off list: >> "Vector reductions don't work on RDNA, as is, but they're >> supposed to be disabled by the insn condition" >> >> This patch disables "fold_left_plus_<mode>", which is about >> vectorization and in the code path shown in the backtrace. >> I can also confirm manually that it fixes the ICE I saw and >> also the ICE for the testfile that Richard's PR shows at the >> end of his backtrace. (-O3 is needed to trigger the ICE.) > > On top of that, OK to push the attached > "GCN: Conditionalize 'define_expand "reduc_<fexpander>_scal_<mode>"' on '!TARGET_RDNA2_PLUS' [PR113615]"? > > Which of the 'assert's are worth keeping? > > Only tested 'vect.exp' for 'check-gcc-c' so far; full testing to run > later. > > Please confirm I'm understanding this correctly: > > Andrew's original commit c7ec7bd1c6590cf4eed267feab490288e0b8d691 > "amdgcn: add -march=gfx1030 EXPERIMENTAL" did this: > > (define_expand "reduc_<reduc_op>_scal_<mode>" > [(set (match_operand:<SCALAR_MODE> 0 "register_operand") > (unspec:<SCALAR_MODE> > [(match_operand:V_ALL 1 "register_operand")] > REDUC_UNSPEC))] > - "" > + "!TARGET_RDNA2" [later '!TARGET_RDNA2_PLUS'] > { > [...] > > This conditional, however, does *not* govern any explicit > 'gen_reduc_plus_scal_<mode>', and therefore Tobias in > commit 7cc2262ec9a410dc56d1c1c6b950c922e14f621d > "gcn/gcn-valu.md: Disable fold_left_plus for TARGET_RDNA2_PLUS [PR113615]" > had to replicate the '!TARGET_RDNA2_PLUS' condition: > >> @@ -4274,7 +4274,8 @@ (define_expand "fold_left_plus_<mode>" >> [(match_operand:<SCALAR_MODE> 0 "register_operand") >> (match_operand:<SCALAR_MODE> 1 "gcn_alu_operand") >> (match_operand:V_FP 2 "gcn_alu_operand")] >> - "can_create_pseudo_p () >> + "!TARGET_RDNA2_PLUS >> + && can_create_pseudo_p () >> && (flag_openacc || flag_openmp >> || flag_associative_math)" >> { > | rtx dest = operands[0]; > | rtx scalar = operands[1]; > | rtx vector = operands[2]; > | rtx tmp = gen_reg_rtx (<SCALAR_MODE>mode); > | > | emit_insn (gen_reduc_plus_scal_<mode> (tmp, vector)); > | [...] > > ..., and I thus now have to do similar for > 'gen_reduc_<expander>_scal_<mode>' use in here: > > (define_expand "reduc_<fexpander>_scal_<mode>" > [(match_operand:<SCALAR_MODE> 0 "register_operand") > (fminmaxop:V_FP > (match_operand:V_FP 1 "register_operand"))] > - "" > + "!TARGET_RDNA2_PLUS" > { > /* fmin/fmax are identical to smin/smax. */ > emit_insn (gen_reduc_<expander>_scal_<mode> (operands[0], operands[1])); > [...] OK. I don't mind the asserts. Hopefully they're redundant, but I suppose it's better than tracking down an unrecognised instruction in a later pass. Andrew
From 1ca37da07f0fd3fa2e87fcbde9f2c2aadbe320dc Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tschwinge@baylibre.com>
Date: Fri, 16 Feb 2024 13:04:00 +0100
Subject: [PATCH] GCN: Conditionalize 'define_expand
"reduc_<fexpander>_scal_<mode>"' on '!TARGET_RDNA2_PLUS' [PR113615]
On top of commit c7ec7bd1c6590cf4eed267feab490288e0b8d691
"amdgcn: add -march=gfx1030 EXPERIMENTAL" conditionalizing
'define_expand "reduc_<reduc_op>_scal_<mode>"' on
'!TARGET_RDNA2' (later: '!TARGET_RDNA2_PLUS'), we then did similar in
commit 7cc2262ec9a410dc56d1c1c6b950c922e14f621d
"gcn/gcn-valu.md: Disable fold_left_plus for TARGET_RDNA2_PLUS [PR113615]"
to conditionalize 'define_expand "fold_left_plus_<mode>"' on
'!TARGET_RDNA2_PLUS', but I found we also need to conditionalize the related
'define_expand "reduc_<fexpander>_scal_<mode>"' on '!TARGET_RDNA2_PLUS', to
avoid ICEs like:
[...]/gcc.dg/vect/pr108608.c: In function 'foo':
[...]/gcc.dg/vect/pr108608.c:9:1: error: unrecognizable insn:
(insn 34 33 35 2 (set (reg:V64DF 723)
(unspec:V64DF [
(reg:V64DF 690 [ vect_m_11.20 ])
(const_int 1 [0x1])
] UNSPEC_MOV_DPP_SHR)) -1
(nil))
during RTL pass: vregs
Similar for 'gcc.dg/vect/vect-fmax-2.c', 'gcc.dg/vect/vect-fmin-2.c', and
'UNSPEC_SMAX_DPP_SHR' for 'gcc.dg/vect/vect-fmax-1.c', and
'UNSPEC_SMIN_DPP_SHR' for 'gcc.dg/vect/vect-fmin-1.c', when running 'vect.exp'
for 'check-gcc-c'.
PR target/113615
gcc/
* config/gcn/gcn-valu.md (define_expand "reduc_<fexpander>_scal_<mode>"):
Conditionalize on '!TARGET_RDNA2_PLUS'.
---
gcc/config/gcn/gcn-valu.md | 6 +++++-
gcc/config/gcn/gcn.cc | 4 ++++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/gcc/config/gcn/gcn-valu.md b/gcc/config/gcn/gcn-valu.md
index 59e27d0aed79..973a72e3fc41 100644
--- a/gcc/config/gcn/gcn-valu.md
+++ b/gcc/config/gcn/gcn-valu.md
@@ -4247,6 +4247,8 @@
REDUC_UNSPEC))]
"!TARGET_RDNA2_PLUS"
{
+ gcc_checking_assert (!TARGET_RDNA2_PLUS);
+
rtx tmp = gcn_expand_reduc_scalar (<MODE>mode, operands[1],
<reduc_unspec>);
@@ -4261,8 +4263,10 @@
[(match_operand:<SCALAR_MODE> 0 "register_operand")
(fminmaxop:V_FP
(match_operand:V_FP 1 "register_operand"))]
- ""
+ "!TARGET_RDNA2_PLUS"
{
+ gcc_checking_assert (!TARGET_RDNA2_PLUS);
+
/* fmin/fmax are identical to smin/smax. */
emit_insn (gen_reduc_<expander>_scal_<mode> (operands[0], operands[1]));
DONE;
diff --git a/gcc/config/gcn/gcn.cc b/gcc/config/gcn/gcn.cc
index fce2d4d30c9d..8fa445deda53 100644
--- a/gcc/config/gcn/gcn.cc
+++ b/gcc/config/gcn/gcn.cc
@@ -5449,6 +5449,8 @@ char *
gcn_expand_dpp_shr_insn (machine_mode mode, const char *insn,
int unspec, int shift)
{
+ gcc_checking_assert (!TARGET_RDNA2_PLUS);
+
static char buf[128];
const char *dpp;
const char *vcc_in = "";
@@ -5510,6 +5512,8 @@ gcn_expand_dpp_shr_insn (machine_mode mode, const char *insn,
rtx
gcn_expand_reduc_scalar (machine_mode mode, rtx src, int unspec)
{
+ gcc_checking_assert (!TARGET_RDNA2_PLUS);
+
machine_mode orig_mode = mode;
machine_mode scalar_mode = GET_MODE_INNER (mode);
int vf = GET_MODE_NUNITS (mode);
--
2.43.0