public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] aarch64: Fix aarch64_ldp_reg_operand predicate not to allow all subreg [PR113221]
@ 2024-01-17  3:29 Andrew Pinski
  2024-01-17  8:59 ` Alex Coplan
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Andrew Pinski @ 2024-01-17  3:29 UTC (permalink / raw)
  To: gcc-patches; +Cc: alex.coplan, Andrew Pinski

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

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] aarch64: Fix aarch64_ldp_reg_operand predicate not to allow all subreg [PR113221]
  2024-01-17  3:29 [PATCH] aarch64: Fix aarch64_ldp_reg_operand predicate not to allow all subreg [PR113221] Andrew Pinski
@ 2024-01-17  8:59 ` Alex Coplan
  2024-01-17 18:54   ` Andrew Pinski (QUIC)
  2024-01-17  9:11 ` Jakub Jelinek
  2024-01-17 10:18 ` Kyrylo Tkachov
  2 siblings, 1 reply; 5+ messages in thread
From: Alex Coplan @ 2024-01-17  8:59 UTC (permalink / raw)
  To: Andrew Pinski; +Cc: gcc-patches

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
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] aarch64: Fix aarch64_ldp_reg_operand predicate not to allow all subreg [PR113221]
  2024-01-17  3:29 [PATCH] aarch64: Fix aarch64_ldp_reg_operand predicate not to allow all subreg [PR113221] Andrew Pinski
  2024-01-17  8:59 ` Alex Coplan
@ 2024-01-17  9:11 ` Jakub Jelinek
  2024-01-17 10:18 ` Kyrylo Tkachov
  2 siblings, 0 replies; 5+ messages in thread
From: Jakub Jelinek @ 2024-01-17  9:11 UTC (permalink / raw)
  To: Andrew Pinski; +Cc: gcc-patches, alex.coplan

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH] aarch64: Fix aarch64_ldp_reg_operand predicate not to allow all subreg [PR113221]
  2024-01-17  3:29 [PATCH] aarch64: Fix aarch64_ldp_reg_operand predicate not to allow all subreg [PR113221] Andrew Pinski
  2024-01-17  8:59 ` Alex Coplan
  2024-01-17  9:11 ` Jakub Jelinek
@ 2024-01-17 10:18 ` Kyrylo Tkachov
  2 siblings, 0 replies; 5+ messages in thread
From: Kyrylo Tkachov @ 2024-01-17 10:18 UTC (permalink / raw)
  To: Andrew Pinski, gcc-patches; +Cc: Alex Coplan



> -----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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH] aarch64: Fix aarch64_ldp_reg_operand predicate not to allow all subreg [PR113221]
  2024-01-17  8:59 ` Alex Coplan
@ 2024-01-17 18:54   ` Andrew Pinski (QUIC)
  0 siblings, 0 replies; 5+ messages in thread
From: Andrew Pinski (QUIC) @ 2024-01-17 18:54 UTC (permalink / raw)
  To: Alex Coplan, Andrew Pinski (QUIC); +Cc: gcc-patches

> -----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
> >

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-01-17 18:54 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-17  3:29 [PATCH] aarch64: Fix aarch64_ldp_reg_operand predicate not to allow all subreg [PR113221] Andrew Pinski
2024-01-17  8:59 ` Alex Coplan
2024-01-17 18:54   ` Andrew Pinski (QUIC)
2024-01-17  9:11 ` Jakub Jelinek
2024-01-17 10:18 ` Kyrylo Tkachov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).