diff mbox series

pr103523: Check for PLUS/MINUS support

Message ID DB9PR08MB66030FB1C55990D025E62326F5719@DB9PR08MB6603.eurprd08.prod.outlook.com
State Committed
Commit a5f65cf7ad640ae398eba7a45c712322ce841809
Headers
Series pr103523: Check for PLUS/MINUS support |

Commit Message

Joel Hutton Dec. 10, 2021, 10:02 a.m. UTC 
  Hi all,

This is to address pr103523.

bootstrapped and regression tested on aarch64.

Check for PLUS_EXPR/MINUS_EXPR support in vectorizable_induction.
PR103523 is an ICE on valid code:

void d(float *a, float b, int c) {
    float e;
    for (; c; c--, e += b)
      a[c] = e;
}

This is due to not checking for PLUS_EXPR support, which is missing in
VNx2sf mode. This causes an ICE at expand time. This patch adds a check
for support in vectorizable_induction.

gcc/ChangeLog:
        
	PR tree-optimization/PR103523
	* tree-vect-loop.c (vectorizable_induction): Check for
    PLUS_EXPR/MINUS_EXPR support.

Comments

Richard Sandiford Dec. 10, 2021, 10:22 a.m. UTC | #1 
Joel Hutton <Joel.Hutton@arm.com> writes:
> Hi all,
>
> This is to address pr103523.
>
> bootstrapped and regression tested on aarch64.
>
> Check for PLUS_EXPR/MINUS_EXPR support in vectorizable_induction.
> PR103523 is an ICE on valid code:
>
> void d(float *a, float b, int c) {
>     float e;
>     for (; c; c--, e += b)
>       a[c] = e;
> }
>
> This is due to not checking for PLUS_EXPR support, which is missing in
> VNx2sf mode. This causes an ICE at expand time. This patch adds a check
> for support in vectorizable_induction.
>
> gcc/ChangeLog:
>
>         PR tree-optimization/PR103523

The bugzilla hook expects: PR tree-optimization/103523

>         * tree-vect-loop.c (vectorizable_induction): Check for
>     PLUS_EXPR/MINUS_EXPR support.

OK, thanks.

Richard
Joel Hutton Dec. 10, 2021, 1:24 p.m. UTC | #2 
ok for backport to 11?
Richard Sandiford Dec. 10, 2021, 6:33 p.m. UTC | #3 
Joel Hutton <Joel.Hutton@arm.com> writes:
> ok for backport to 11?

Yes, thanks.

Richard
Tobias Burnus Dec. 13, 2021, 2:27 p.m. UTC | #4 
Hi Joel,

your patch fails here with:

../../repos/gcc-11-commit/gcc/tree-vect-loop.c:8000:8: error:
‘directly_supported_p’ was not declared in this scope
  8000 |   if (!directly_supported_p (PLUS_EXPR, step_vectype)
       |        ^~~~~~~~~~~~~~~~~~~~

And "git grep" shows that this is only present in:

gcc/tree-vect-loop.c:  if (!directly_supported_p (PLUS_EXPR, step_vectype)
gcc/tree-vect-loop.c:      || !directly_supported_p (MINUS_EXPR,
step_vectype))

That's different on mainline, which offers that function.

Tobias

On 10.12.21 14:24, Joel Hutton via Gcc-patches wrote:
> ok for backport to 11?
> ________________________________
> From: Richard Sandiford <richard.sandiford@arm.com>
> Sent: 10 December 2021 10:22
> To: Joel Hutton <Joel.Hutton@arm.com>
> Cc: GCC Patches <gcc-patches@gcc.gnu.org>; Richard Biener <rguenther@suse.de>
> Subject: Re: pr103523: Check for PLUS/MINUS support
>
> Joel Hutton <Joel.Hutton@arm.com> writes:
>> Hi all,
>>
>> This is to address pr103523.
>>
>> bootstrapped and regression tested on aarch64.
>>
>> Check for PLUS_EXPR/MINUS_EXPR support in vectorizable_induction.
>> PR103523 is an ICE on valid code:
>>
>> void d(float *a, float b, int c) {
>>      float e;
>>      for (; c; c--, e += b)
>>        a[c] = e;
>> }
>>
>> This is due to not checking for PLUS_EXPR support, which is missing in
>> VNx2sf mode. This causes an ICE at expand time. This patch adds a check
>> for support in vectorizable_induction.
>>
>> gcc/ChangeLog:
>>
>>          PR tree-optimization/PR103523
> The bugzilla hook expects: PR tree-optimization/103523
>
>>          * tree-vect-loop.c (vectorizable_induction): Check for
>>      PLUS_EXPR/MINUS_EXPR support.
> OK, thanks.
>
> Richard
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
Richard Biener Dec. 13, 2021, 2:47 p.m. UTC | #5 
On December 13, 2021 3:27:50 PM GMT+01:00, Tobias Burnus <tobias@codesourcery.com> wrote:
>Hi Joel,
>
>your patch fails here with:
>
>../../repos/gcc-11-commit/gcc/tree-vect-loop.c:8000:8: error:
>‘directly_supported_p’ was not declared in this scope
>  8000 |   if (!directly_supported_p (PLUS_EXPR, step_vectype)
>       |        ^~~~~~~~~~~~~~~~~~~~
>
>And "git grep" shows that this is only present in:
>
>gcc/tree-vect-loop.c:  if (!directly_supported_p (PLUS_EXPR, step_vectype)
>gcc/tree-vect-loop.c:      || !directly_supported_p (MINUS_EXPR,
>step_vectype))
>
>That's different on mainline, which offers that function.

Just as a reminder, backports need regular bootstrap and regtest validation on the respective branches. 

Richard. 

>Tobias
>
>On 10.12.21 14:24, Joel Hutton via Gcc-patches wrote:
>> ok for backport to 11?
>> ________________________________
>> From: Richard Sandiford <richard.sandiford@arm.com>
>> Sent: 10 December 2021 10:22
>> To: Joel Hutton <Joel.Hutton@arm.com>
>> Cc: GCC Patches <gcc-patches@gcc.gnu.org>; Richard Biener <rguenther@suse.de>
>> Subject: Re: pr103523: Check for PLUS/MINUS support
>>
>> Joel Hutton <Joel.Hutton@arm.com> writes:
>>> Hi all,
>>>
>>> This is to address pr103523.
>>>
>>> bootstrapped and regression tested on aarch64.
>>>
>>> Check for PLUS_EXPR/MINUS_EXPR support in vectorizable_induction.
>>> PR103523 is an ICE on valid code:
>>>
>>> void d(float *a, float b, int c) {
>>>      float e;
>>>      for (; c; c--, e += b)
>>>        a[c] = e;
>>> }
>>>
>>> This is due to not checking for PLUS_EXPR support, which is missing in
>>> VNx2sf mode. This causes an ICE at expand time. This patch adds a check
>>> for support in vectorizable_induction.
>>>
>>> gcc/ChangeLog:
>>>
>>>          PR tree-optimization/PR103523
>> The bugzilla hook expects: PR tree-optimization/103523
>>
>>>          * tree-vect-loop.c (vectorizable_induction): Check for
>>>      PLUS_EXPR/MINUS_EXPR support.
>> OK, thanks.
>>
>> Richard
>-----------------
>Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
Joel Hutton Dec. 13, 2021, 3:02 p.m. UTC | #6 
My mistake, reworked patch. Tests are still running.
Joel Hutton Dec. 14, 2021, 9:37 a.m. UTC | #7 
Bootstrapped and regression tested on releases/gcc-11 on aarch64.

Ok for 11?

Previous commit broke build as it relied on directly_supported_p which
is not in 11. This reworks to avoid using directly_supported_p.

gcc/ChangeLog:

              PR bootstrap/103688
              * tree-vect-loop.c (vectorizable_induction): Rework to avoid
    directly_supported_p

From: Joel Hutton <Joel.Hutton@arm.com>
Sent: 13 December 2021 15:02
To: Richard Biener <richard.guenther@gmail.com>; gcc-patches@gcc.gnu.org; Tobias Burnus <tobias@codesourcery.com>; Richard Sandiford <Richard.Sandiford@arm.com>
Cc: Richard Biener <rguenther@suse.de>
Subject: Re: GCC 11 backport does not build (no "directly_supported_p") - was: Re: pr103523: Check for PLUS/MINUS support

My mistake, reworked patch. Tests are still running.
Jakub Jelinek Dec. 14, 2021, 9:53 a.m. UTC | #8 
On Tue, Dec 14, 2021 at 09:37:03AM +0000, Joel Hutton via Gcc-patches wrote:
> Bootstrapped and regression tested on releases/gcc-11 on aarch64.
> 
> Ok for 11?
> 
> Previous commit broke build as it relied on directly_supported_p which
> is not in 11. This reworks to avoid using directly_supported_p.
> 
> gcc/ChangeLog:
> 
>               PR bootstrap/103688
>               * tree-vect-loop.c (vectorizable_induction): Rework to avoid
>     directly_supported_p

Missing . after directly_supported_p

--- a/gcc/tree-vect-loop.c
+++ b/gcc/tree-vect-loop.c
@@ -7997,8 +7997,14 @@ vectorizable_induction (loop_vec_info loop_vinfo,
   tree step_vectype = get_same_sized_vectype (TREE_TYPE (step_expr), vectype);
 
   /* Check for backend support of PLUS/MINUS_EXPR. */
-  if (!directly_supported_p (PLUS_EXPR, step_vectype)
-      || !directly_supported_p (MINUS_EXPR, step_vectype))
+  direct_optab ot_plus = optab_for_tree_code (tree_code (PLUS_EXPR),
+						 step_vectype, optab_default);
+  direct_optab ot_minus = optab_for_tree_code (tree_code (MINUS_EXPR),
+						 step_vectype, optab_default);

Why tree_code (PLUS_EXPR) instead of just PLUS_EXPR (ditto MINUS_EXPR)?
The formatting is off, step_vectype isn't aligned below tree_code.

+  if (ot_plus == unknown_optab
+      || ot_minus == unknown_optab
+      || optab_handler (ot_minus, TYPE_MODE (step_vectype)) == CODE_FOR_nothing
+      || optab_handler (ot_plus, TYPE_MODE (step_vectype)) == CODE_FOR_nothing)
     return false;

Won't optab_handler just return CODE_FOR_nothing for unknown_optab?

Anyway, I think best would be to write it as:
  if (!target_supports_op_p (step_vectype, PLUS_EXPR, optab_default)
      || !target_supports_op_p (step_vectype, MINUS_EXPR, optab_default))
    return false;

	Jakub
Joel Hutton Dec. 14, 2021, 10:46 a.m. UTC | #9 
> +  if (ot_plus == unknown_optab
> +      || ot_minus == unknown_optab
> +      || optab_handler (ot_minus, TYPE_MODE (step_vectype)) ==
> CODE_FOR_nothing
> +      || optab_handler (ot_plus, TYPE_MODE (step_vectype)) ==
> + CODE_FOR_nothing)
>      return false;
> 
> Won't optab_handler just return CODE_FOR_nothing for unknown_optab?

I was taking the check used in directly_supported_p

return (optab != unknown_optab$
 	&& optab_handler (optab, TYPE_MODE (type)) != CODE_FOR_nothing);$

> Anyway, I think best would be to write it as:
>   if (!target_supports_op_p (step_vectype, PLUS_EXPR, optab_default)
>       || !target_supports_op_p (step_vectype, MINUS_EXPR, optab_default))
>     return false;
Looks good to me.

Patch attached.

Tests running on gcc-11 on aarch64. 

Ok for 11 once tests come back?
Jakub Jelinek Dec. 14, 2021, 10:57 a.m. UTC | #10 
On Tue, Dec 14, 2021 at 10:46:39AM +0000, Joel Hutton wrote:
> > +  if (ot_plus == unknown_optab
> > +      || ot_minus == unknown_optab
> > +      || optab_handler (ot_minus, TYPE_MODE (step_vectype)) ==
> > CODE_FOR_nothing
> > +      || optab_handler (ot_plus, TYPE_MODE (step_vectype)) ==
> > + CODE_FOR_nothing)
> >      return false;
> > 
> > Won't optab_handler just return CODE_FOR_nothing for unknown_optab?
> 
> I was taking the check used in directly_supported_p
> 
> return (optab != unknown_optab$
>  	&& optab_handler (optab, TYPE_MODE (type)) != CODE_FOR_nothing);$
> 
> > Anyway, I think best would be to write it as:
> >   if (!target_supports_op_p (step_vectype, PLUS_EXPR, optab_default)
> >       || !target_supports_op_p (step_vectype, MINUS_EXPR, optab_default))
> >     return false;
> Looks good to me.
> 
> Patch attached.
> 
> Tests running on gcc-11 on aarch64. 
> 
> Ok for 11 once tests come back?

Yes, thanks.

	Jakub
diff mbox series

Patch

diff --git a/gcc/testsuite/gcc.target/aarch64/pr103523.c b/gcc/testsuite/gcc.target/aarch64/pr103523.c
new file mode 100644
index 0000000000000000000000000000000000000000..736e8936c5f6768bdf098ddc37b2c21ab74ee0df
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr103523.c
@@ -0,0 +1,8 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=armv8-a+sve -mtune=neoverse-v1 -Ofast" } */
+
+void d(float *a, float b, int c) {
+    float e;
+    for (; c; c--, e += b)
+      a[c] = e;
+}
diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
index 7f544ba1fd5198dd32cda05e62382ab2e1e9bb50..f700d5e7ac2c05402407a46113320f79359906fa 100644
--- a/gcc/tree-vect-loop.c
+++ b/gcc/tree-vect-loop.c
@@ -8065,6 +8065,15 @@  vectorizable_induction (loop_vec_info loop_vinfo,
       return false;
     }
 
+  step_expr = STMT_VINFO_LOOP_PHI_EVOLUTION_PART (stmt_info);
+  gcc_assert (step_expr != NULL_TREE);
+  tree step_vectype = get_same_sized_vectype (TREE_TYPE (step_expr), vectype);
+
+  /* Check for backend support of PLUS/MINUS_EXPR. */
+  if (!directly_supported_p (PLUS_EXPR, step_vectype)
+      || !directly_supported_p (MINUS_EXPR, step_vectype))
+    return false;
+
   if (!vec_stmt) /* transformation not required.  */
     {
       unsigned inside_cost = 0, prologue_cost = 0;
@@ -8124,10 +8133,6 @@  vectorizable_induction (loop_vec_info loop_vinfo,
   if (dump_enabled_p ())
     dump_printf_loc (MSG_NOTE, vect_location, "transform induction phi.\n");
 
-  step_expr = STMT_VINFO_LOOP_PHI_EVOLUTION_PART (stmt_info);
-  gcc_assert (step_expr != NULL_TREE);
-  tree step_vectype = get_same_sized_vectype (TREE_TYPE (step_expr), vectype);
-
   pe = loop_preheader_edge (iv_loop);
   /* Find the first insertion point in the BB.  */
   basic_block bb = gimple_bb (phi);