> -----Original Message-----
> From: Alex Coplan <alex.cop...@arm.com>
> Sent: Wednesday, January 17, 2024 12:59 AM
> To: Andrew Pinski (QUIC) <quic_apin...@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_apin...@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
> >
