diff mbox series

aarch64: Fix aarch64_ldp_reg_operand predicate not to allow all subreg [PR113221]

Message ID 20240117032904.80831-1-quic_apinski@quicinc.com
State New
Headers
Series aarch64: Fix aarch64_ldp_reg_operand predicate not to allow all subreg [PR113221] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Testing passed

Commit Message

Andrew Pinski Jan. 17, 2024, 3:29 a.m. UTC 
  So the problem here is that aarch64_ldp_reg_operand will all subreg even subreg of lo_sum.
When LRA tries to fix that up, all things break. So the fix is to change the check to only
allow reg and subreg of regs.

Note the tendancy here is to use register_operand but that checks the mode of the register
but we need to allow a mismatch modes for this predicate for now.

Built and tested for aarch64-linux-gnu with no regressions
(Also tested with the LD/ST pair pass back on).

	PR target/113221

gcc/ChangeLog:

	* config/aarch64/predicates.md (aarch64_ldp_reg_operand): For subreg,
	only allow REG operands isntead of allowing all.

gcc/testsuite/ChangeLog:

	* gcc.c-torture/compile/pr113221-1.c: New test.

Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
---
 gcc/config/aarch64/predicates.md                 |  8 +++++++-
 gcc/testsuite/gcc.c-torture/compile/pr113221-1.c | 12 ++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr113221-1.c

Comments

Alex Coplan Jan. 17, 2024, 8:59 a.m. UTC | #1 
Hi Andrew,

On 16/01/2024 19:29, Andrew Pinski wrote:
> So the problem here is that aarch64_ldp_reg_operand will all subreg even subreg of lo_sum.
> When LRA tries to fix that up, all things break. So the fix is to change the check to only
> allow reg and subreg of regs.

Thanks a lot for tracking this down, I really appreciate having some help with
the bug-fixing.  Sorry for not getting to it sooner myself, I'm working on
PR113089 which ended up taking longer than expected to fix.

> 
> Note the tendancy here is to use register_operand but that checks the mode of the register
> but we need to allow a mismatch modes for this predicate for now.

Yeah, due to the design of the patterns using special predicates we need
to allow a mode mismatch with the contextual mode.

The patch broadly LGTM (although I can't approve), but I've left a
couple of minor comments below.

> 
> Built and tested for aarch64-linux-gnu with no regressions
> (Also tested with the LD/ST pair pass back on).
> 
> 	PR target/113221
> 
> gcc/ChangeLog:
> 
> 	* config/aarch64/predicates.md (aarch64_ldp_reg_operand): For subreg,
> 	only allow REG operands isntead of allowing all.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.c-torture/compile/pr113221-1.c: New test.
> 
> Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
> ---
>  gcc/config/aarch64/predicates.md                 |  8 +++++++-
>  gcc/testsuite/gcc.c-torture/compile/pr113221-1.c | 12 ++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr113221-1.c
> 
> diff --git a/gcc/config/aarch64/predicates.md b/gcc/config/aarch64/predicates.md
> index 8a204e48bb5..256268517d8 100644
> --- a/gcc/config/aarch64/predicates.md
> +++ b/gcc/config/aarch64/predicates.md
> @@ -313,7 +313,13 @@ (define_predicate "pmode_plus_operator"
>  
>  (define_special_predicate "aarch64_ldp_reg_operand"
>    (and
> -    (match_code "reg,subreg")
> +    (ior
> +      (match_code "reg")
> +      (and
> +       (match_code "subreg")
> +       (match_test "GET_CODE (SUBREG_REG (op)) == REG")

This could be just REG_P (SUBREG_REG (op)) in the match_test.

> +      )
> +    )

I think it would be more in keeping with the style in the rest of the file to
have the closing parens on the same line as the SUBREG_REG match_test.

>      (match_test "aarch64_ldpstp_operand_mode_p (GET_MODE (op))")
>      (ior
>        (match_test "mode == VOIDmode")
> diff --git a/gcc/testsuite/gcc.c-torture/compile/pr113221-1.c b/gcc/testsuite/gcc.c-torture/compile/pr113221-1.c
> new file mode 100644
> index 00000000000..152a510786e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/compile/pr113221-1.c
> @@ -0,0 +1,12 @@
> +/* { dg-options "-fno-move-loop-invariants -funroll-all-loops" } */

Does this need to be dg-additional-options?  Naively I would expect the
dg-options clause to override the torture options (and potentially any
options provided in RUNTESTFLAGS, e.g. to re-enable the ldp/stp pass).

Thanks again for the patch, and apologies for the oversight on my part: I'd
missed that register_operand also checks the code inside the subreg.

Alex

> +/* PR target/113221 */
> +/* This used to ICE after the `load/store pair fusion pass` was added
> +   due to the predicate aarch64_ldp_reg_operand allowing too much. */
> +
> +
> +void bar();
> +void foo(int* b) {
> +  for (;;)
> +    *b++ = (long)bar;
> +}
> +
> -- 
> 2.39.3
>
Jakub Jelinek Jan. 17, 2024, 9:11 a.m. UTC | #2 
On Tue, Jan 16, 2024 at 07:29:04PM -0800, Andrew Pinski wrote:
> So the problem here is that aarch64_ldp_reg_operand will all subreg even subreg of lo_sum.
> When LRA tries to fix that up, all things break. So the fix is to change the check to only
> allow reg and subreg of regs.
> 
> Note the tendancy here is to use register_operand but that checks the mode of the register
> but we need to allow a mismatch modes for this predicate for now.
> 
> Built and tested for aarch64-linux-gnu with no regressions
> (Also tested with the LD/ST pair pass back on).
> 
> 	PR target/113221
> 
> gcc/ChangeLog:
> 
> 	* config/aarch64/predicates.md (aarch64_ldp_reg_operand): For subreg,
> 	only allow REG operands isntead of allowing all.

s/isntead/instead/
Otherwise I defer to AArch64 maintainers.

	Jakub
Kyrylo Tkachov Jan. 17, 2024, 10:18 a.m. UTC | #3 
> -----Original Message-----
> From: Andrew Pinski <quic_apinski@quicinc.com>
> Sent: Wednesday, January 17, 2024 3:29 AM
> To: gcc-patches@gcc.gnu.org
> Cc: Alex Coplan <Alex.Coplan@arm.com>; Andrew Pinski
> <quic_apinski@quicinc.com>
> Subject: [PATCH] aarch64: Fix aarch64_ldp_reg_operand predicate not to allow
> all subreg [PR113221]
> 
> So the problem here is that aarch64_ldp_reg_operand will all subreg even subreg
> of lo_sum.
> When LRA tries to fix that up, all things break. So the fix is to change the check to
> only
> allow reg and subreg of regs.
> 
> Note the tendancy here is to use register_operand but that checks the mode of
> the register
> but we need to allow a mismatch modes for this predicate for now.
> 
> Built and tested for aarch64-linux-gnu with no regressions
> (Also tested with the LD/ST pair pass back on).

Ok with the comments from Alex addressed.
Thanks,
Kyrill

> 
> 	PR target/113221
> 
> gcc/ChangeLog:
> 
> 	* config/aarch64/predicates.md (aarch64_ldp_reg_operand): For subreg,
> 	only allow REG operands isntead of allowing all.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.c-torture/compile/pr113221-1.c: New test.
> 
> Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
> ---
>  gcc/config/aarch64/predicates.md                 |  8 +++++++-
>  gcc/testsuite/gcc.c-torture/compile/pr113221-1.c | 12 ++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr113221-1.c
> 
> diff --git a/gcc/config/aarch64/predicates.md
> b/gcc/config/aarch64/predicates.md
> index 8a204e48bb5..256268517d8 100644
> --- a/gcc/config/aarch64/predicates.md
> +++ b/gcc/config/aarch64/predicates.md
> @@ -313,7 +313,13 @@ (define_predicate "pmode_plus_operator"
> 
>  (define_special_predicate "aarch64_ldp_reg_operand"
>    (and
> -    (match_code "reg,subreg")
> +    (ior
> +      (match_code "reg")
> +      (and
> +       (match_code "subreg")
> +       (match_test "GET_CODE (SUBREG_REG (op)) == REG")
> +      )
> +    )
>      (match_test "aarch64_ldpstp_operand_mode_p (GET_MODE (op))")
>      (ior
>        (match_test "mode == VOIDmode")
> diff --git a/gcc/testsuite/gcc.c-torture/compile/pr113221-1.c
> b/gcc/testsuite/gcc.c-torture/compile/pr113221-1.c
> new file mode 100644
> index 00000000000..152a510786e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/compile/pr113221-1.c
> @@ -0,0 +1,12 @@
> +/* { dg-options "-fno-move-loop-invariants -funroll-all-loops" } */
> +/* PR target/113221 */
> +/* This used to ICE after the `load/store pair fusion pass` was added
> +   due to the predicate aarch64_ldp_reg_operand allowing too much. */
> +
> +
> +void bar();
> +void foo(int* b) {
> +  for (;;)
> +    *b++ = (long)bar;
> +}
> +
> --
> 2.39.3
Andrew Pinski Jan. 17, 2024, 6:54 p.m. UTC | #4 
> -----Original Message-----
> From: Alex Coplan <alex.coplan@arm.com>
> Sent: Wednesday, January 17, 2024 12:59 AM
> To: Andrew Pinski (QUIC) <quic_apinski@quicinc.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] aarch64: Fix aarch64_ldp_reg_operand predicate not to
> allow all subreg [PR113221]
> 
> Hi Andrew,
> 
> On 16/01/2024 19:29, Andrew Pinski wrote:
> > So the problem here is that aarch64_ldp_reg_operand will all subreg even
> subreg of lo_sum.
> > When LRA tries to fix that up, all things break. So the fix is to
> > change the check to only allow reg and subreg of regs.
> 
> Thanks a lot for tracking this down, I really appreciate having some help with
> the bug-fixing.  Sorry for not getting to it sooner myself, I'm working on
> PR113089 which ended up taking longer than expected to fix.
> 
> >
> > Note the tendancy here is to use register_operand but that checks the
> > mode of the register but we need to allow a mismatch modes for this
> predicate for now.
> 
> Yeah, due to the design of the patterns using special predicates we need to
> allow a mode mismatch with the contextual mode.
> 
> The patch broadly LGTM (although I can't approve), but I've left a couple of
> minor comments below.
> 
> >
> > Built and tested for aarch64-linux-gnu with no regressions (Also
> > tested with the LD/ST pair pass back on).
> >
> > 	PR target/113221
> >
> > gcc/ChangeLog:
> >
> > 	* config/aarch64/predicates.md (aarch64_ldp_reg_operand): For
> subreg,
> > 	only allow REG operands isntead of allowing all.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 	* gcc.c-torture/compile/pr113221-1.c: New test.
> >
> > Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
> > ---
> >  gcc/config/aarch64/predicates.md                 |  8 +++++++-
> >  gcc/testsuite/gcc.c-torture/compile/pr113221-1.c | 12 ++++++++++++
> >  2 files changed, 19 insertions(+), 1 deletion(-)  create mode 100644
> > gcc/testsuite/gcc.c-torture/compile/pr113221-1.c
> >
> > diff --git a/gcc/config/aarch64/predicates.md
> > b/gcc/config/aarch64/predicates.md
> > index 8a204e48bb5..256268517d8 100644
> > --- a/gcc/config/aarch64/predicates.md
> > +++ b/gcc/config/aarch64/predicates.md
> > @@ -313,7 +313,13 @@ (define_predicate "pmode_plus_operator"
> >
> >  (define_special_predicate "aarch64_ldp_reg_operand"
> >    (and
> > -    (match_code "reg,subreg")
> > +    (ior
> > +      (match_code "reg")
> > +      (and
> > +       (match_code "subreg")
> > +       (match_test "GET_CODE (SUBREG_REG (op)) == REG")
> 
> This could be just REG_P (SUBREG_REG (op)) in the match_test.
> 
> > +      )
> > +    )
> 
> I think it would be more in keeping with the style in the rest of the file to have
> the closing parens on the same line as the SUBREG_REG match_test.
> 
> >      (match_test "aarch64_ldpstp_operand_mode_p (GET_MODE (op))")
> >      (ior
> >        (match_test "mode == VOIDmode") diff --git
> > a/gcc/testsuite/gcc.c-torture/compile/pr113221-1.c
> > b/gcc/testsuite/gcc.c-torture/compile/pr113221-1.c
> > new file mode 100644
> > index 00000000000..152a510786e
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.c-torture/compile/pr113221-1.c
> > @@ -0,0 +1,12 @@
> > +/* { dg-options "-fno-move-loop-invariants -funroll-all-loops" } */
> 
> Does this need to be dg-additional-options?  Naively I would expect the dg-
> options clause to override the torture options (and potentially any options
> provided in RUNTESTFLAGS, e.g. to re-enable the ldp/stp pass).

I just checked my testsuite run and the answer for this is no it does not need to be dg-additional-options in this case.
dg-options does not override the torture options but rather puts them after those ones. 
As far as I understand it, dg-additional-options makes it easier to have different options added per target but in this case we don't need that.

Will update the patch with the rest of the changes and push it in next few hours.
I did notice an issue with the testcase though, I need to cast to __SIZE_TYPE__ instead of long to allow it to work with targets that are not ILP32 and LP54. I will fix that too.

Thanks,
Andrew Pinski

> 
> Thanks again for the patch, and apologies for the oversight on my part: I'd
> missed that register_operand also checks the code inside the subreg.
> 
> Alex
> 
> > +/* PR target/113221 */
> > +/* This used to ICE after the `load/store pair fusion pass` was added
> > +   due to the predicate aarch64_ldp_reg_operand allowing too much. */
> > +
> > +
> > +void bar();
> > +void foo(int* b) {
> > +  for (;;)
> > +    *b++ = (long)bar;
> > +}
> > +
> > --
> > 2.39.3
> >
diff mbox series

Patch

diff --git a/gcc/config/aarch64/predicates.md b/gcc/config/aarch64/predicates.md
index 8a204e48bb5..256268517d8 100644
--- a/gcc/config/aarch64/predicates.md
+++ b/gcc/config/aarch64/predicates.md
@@ -313,7 +313,13 @@  (define_predicate "pmode_plus_operator"
 
 (define_special_predicate "aarch64_ldp_reg_operand"
   (and
-    (match_code "reg,subreg")
+    (ior
+      (match_code "reg")
+      (and
+       (match_code "subreg")
+       (match_test "GET_CODE (SUBREG_REG (op)) == REG")
+      )
+    )
     (match_test "aarch64_ldpstp_operand_mode_p (GET_MODE (op))")
     (ior
       (match_test "mode == VOIDmode")
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr113221-1.c b/gcc/testsuite/gcc.c-torture/compile/pr113221-1.c
new file mode 100644
index 00000000000..152a510786e
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr113221-1.c
@@ -0,0 +1,12 @@ 
+/* { dg-options "-fno-move-loop-invariants -funroll-all-loops" } */
+/* PR target/113221 */
+/* This used to ICE after the `load/store pair fusion pass` was added
+   due to the predicate aarch64_ldp_reg_operand allowing too much. */
+
+
+void bar();
+void foo(int* b) {
+  for (;;)
+    *b++ = (long)bar;
+}
+