diff mbox 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])

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

Commit Message

Thomas Schwinge Feb. 16, 2024, 2:34 p.m. UTC
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]));
     [...]


Grüße
 Thomas

Comments

Andrew Stubbs Feb. 16, 2024, 2:39 p.m. UTC | #1
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
diff mbox series

Patch

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