diff mbox series

aarch64: Validate register operands early in ldp fusion pass [PR113062]

Message ID ZYFgN6TSlhCDc6xA@arm.com
State New
Headers show
Series aarch64: Validate register operands early in ldp fusion pass [PR113062] | expand

Commit Message

Alex Coplan Dec. 19, 2023, 9:19 a.m. UTC 
We were missing validation of the candidate register operands in the
ldp/stp pass.  I was relying on recog rejecting such cases when we
formed the final pair insn, but the testcase shows that with
-fharden-conditionals we attempt to combine two insns with asm_operands,
both containing mem rtxes.  This then trips the assert:

gcc_assert (change->new_uses.is_valid ());

in the stp case as we aren't expecting to have (distinct) uses of mem in
the candidate stores.

Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk?

Thanks,
Alex

gcc/ChangeLog:

	PR target/113062
	* config/aarch64/aarch64-ldp-fusion.cc
	(ldp_bb_info::track_access): Punt on accesses with invalid
	register operands.

gcc/testsuite/ChangeLog:

	PR target/113062
	* gcc.dg/pr113062.c: New test.

Comments

Richard Sandiford Dec. 19, 2023, 10:15 a.m. UTC | #1 
Alex Coplan <alex.coplan@arm.com> writes:
> We were missing validation of the candidate register operands in the
> ldp/stp pass.  I was relying on recog rejecting such cases when we
> formed the final pair insn, but the testcase shows that with
> -fharden-conditionals we attempt to combine two insns with asm_operands,
> both containing mem rtxes.  This then trips the assert:
>
> gcc_assert (change->new_uses.is_valid ());
>
> in the stp case as we aren't expecting to have (distinct) uses of mem in
> the candidate stores.
>
> Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk?
>
> Thanks,
> Alex
>
> gcc/ChangeLog:
>
> 	PR target/113062
> 	* config/aarch64/aarch64-ldp-fusion.cc
> 	(ldp_bb_info::track_access): Punt on accesses with invalid
> 	register operands.
>
> gcc/testsuite/ChangeLog:
>
> 	PR target/113062
> 	* gcc.dg/pr113062.c: New test.
>
> diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc b/gcc/config/aarch64/aarch64-ldp-fusion.cc
> index 327ba4e417d..273db8c582f 100644
> --- a/gcc/config/aarch64/aarch64-ldp-fusion.cc
> +++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc
> @@ -476,6 +476,12 @@ ldp_bb_info::track_access (insn_info *insn, bool load_p, rtx mem)
>  
>    const lfs_fields lfs = { load_p, fpsimd_op_p, mem_size };
>  
> +  // Ignore the access if the register operand isn't suitable for ldp/stp.
> +  if (!REG_P (reg_op)
> +      && !SUBREG_P (reg_op)
> +      && (load_p || !aarch64_const_zero_rtx_p (reg_op)))
> +    return;
> +

It might be more natural to test this before:

  // We want to segregate FP/SIMD accesses from GPR accesses.
  //
  // Before RA, we use the modes, noting that stores of constant zero
  // operands use GPRs (even in non-integer modes).  After RA, we use
  // the hard register numbers.
  const bool fpsimd_op_p
    = reload_completed
    ? (REG_P (reg_op) && FP_REGNUM_P (REGNO (reg_op)))
    : (GET_MODE_CLASS (mem_mode) != MODE_INT
       && (load_p || !aarch64_const_zero_rtx_p (reg_op)));

so that that code is running with a pre-checked operand.

Also, how about using the predicates instead:

  if (load_p
      ? !aarch64_ldp_reg_operand (reg_op, VOIDmode)
      : !aarch64_stp_reg_operand (reg_op, VOIDmode))
    return;

OK with those changes, or without if you prefer.

Thanks,
Richard

>    if (track_via_mem_expr (insn, mem, lfs))
>      return;
>  
> diff --git a/gcc/testsuite/gcc.dg/pr113062.c b/gcc/testsuite/gcc.dg/pr113062.c
> new file mode 100644
> index 00000000000..5667c17b0f6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr113062.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Oz -fharden-conditional-branches" } */
> +long double foo;
> +double bar;
> +void abort();
> +void check() {
> +  if (foo == bar)
> +    abort();
> +}
> +
Alex Coplan Dec. 19, 2023, 10:34 a.m. UTC | #2 
On 19/12/2023 10:15, Richard Sandiford wrote:
> Alex Coplan <alex.coplan@arm.com> writes:
> > We were missing validation of the candidate register operands in the
> > ldp/stp pass.  I was relying on recog rejecting such cases when we
> > formed the final pair insn, but the testcase shows that with
> > -fharden-conditionals we attempt to combine two insns with asm_operands,
> > both containing mem rtxes.  This then trips the assert:
> >
> > gcc_assert (change->new_uses.is_valid ());
> >
> > in the stp case as we aren't expecting to have (distinct) uses of mem in
> > the candidate stores.
> >
> > Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk?
> >
> > Thanks,
> > Alex
> >
> > gcc/ChangeLog:
> >
> > 	PR target/113062
> > 	* config/aarch64/aarch64-ldp-fusion.cc
> > 	(ldp_bb_info::track_access): Punt on accesses with invalid
> > 	register operands.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 	PR target/113062
> > 	* gcc.dg/pr113062.c: New test.
> >
> > diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc b/gcc/config/aarch64/aarch64-ldp-fusion.cc
> > index 327ba4e417d..273db8c582f 100644
> > --- a/gcc/config/aarch64/aarch64-ldp-fusion.cc
> > +++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc
> > @@ -476,6 +476,12 @@ ldp_bb_info::track_access (insn_info *insn, bool load_p, rtx mem)
> >  
> >    const lfs_fields lfs = { load_p, fpsimd_op_p, mem_size };
> >  
> > +  // Ignore the access if the register operand isn't suitable for ldp/stp.
> > +  if (!REG_P (reg_op)
> > +      && !SUBREG_P (reg_op)
> > +      && (load_p || !aarch64_const_zero_rtx_p (reg_op)))
> > +    return;
> > +
> 
> It might be more natural to test this before:
> 
>   // We want to segregate FP/SIMD accesses from GPR accesses.
>   //
>   // Before RA, we use the modes, noting that stores of constant zero
>   // operands use GPRs (even in non-integer modes).  After RA, we use
>   // the hard register numbers.
>   const bool fpsimd_op_p
>     = reload_completed
>     ? (REG_P (reg_op) && FP_REGNUM_P (REGNO (reg_op)))
>     : (GET_MODE_CLASS (mem_mode) != MODE_INT
>        && (load_p || !aarch64_const_zero_rtx_p (reg_op)));
> 
> so that that code is running with a pre-checked operand.

Yeah, I agree that seems a bit more natural, I'll move the check up.

> 
> Also, how about using the predicates instead:
> 
>   if (load_p
>       ? !aarch64_ldp_reg_operand (reg_op, VOIDmode)
>       : !aarch64_stp_reg_operand (reg_op, VOIDmode))
>     return;

I thought about doing that, but it seems that we'd effectively just be
re-doing the mode check we did above by calling ldp_operand_mode_ok_p
(assuming generic RTL rules hold), so it seems a bit wasteful to call
the predicates.  Given that this function is called on every (single
set) memory access in a function, I wonder if we should prefer the
inline check?

Thanks,
Alex

> 
> OK with those changes, or without if you prefer.
> 
> Thanks,
> Richard
> 
> >    if (track_via_mem_expr (insn, mem, lfs))
> >      return;
> >  
> > diff --git a/gcc/testsuite/gcc.dg/pr113062.c b/gcc/testsuite/gcc.dg/pr113062.c
> > new file mode 100644
> > index 00000000000..5667c17b0f6
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/pr113062.c
> > @@ -0,0 +1,10 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-Oz -fharden-conditional-branches" } */
> > +long double foo;
> > +double bar;
> > +void abort();
> > +void check() {
> > +  if (foo == bar)
> > +    abort();
> > +}
> > +
Richard Sandiford Dec. 19, 2023, 1:38 p.m. UTC | #3 
Alex Coplan <alex.coplan@arm.com> writes:
> On 19/12/2023 10:15, Richard Sandiford wrote:
>> Alex Coplan <alex.coplan@arm.com> writes:
>> > We were missing validation of the candidate register operands in the
>> > ldp/stp pass.  I was relying on recog rejecting such cases when we
>> > formed the final pair insn, but the testcase shows that with
>> > -fharden-conditionals we attempt to combine two insns with asm_operands,
>> > both containing mem rtxes.  This then trips the assert:
>> >
>> > gcc_assert (change->new_uses.is_valid ());
>> >
>> > in the stp case as we aren't expecting to have (distinct) uses of mem in
>> > the candidate stores.
>> >
>> > Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk?
>> >
>> > Thanks,
>> > Alex
>> >
>> > gcc/ChangeLog:
>> >
>> > 	PR target/113062
>> > 	* config/aarch64/aarch64-ldp-fusion.cc
>> > 	(ldp_bb_info::track_access): Punt on accesses with invalid
>> > 	register operands.
>> >
>> > gcc/testsuite/ChangeLog:
>> >
>> > 	PR target/113062
>> > 	* gcc.dg/pr113062.c: New test.
>> >
>> > diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc b/gcc/config/aarch64/aarch64-ldp-fusion.cc
>> > index 327ba4e417d..273db8c582f 100644
>> > --- a/gcc/config/aarch64/aarch64-ldp-fusion.cc
>> > +++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc
>> > @@ -476,6 +476,12 @@ ldp_bb_info::track_access (insn_info *insn, bool load_p, rtx mem)
>> >  
>> >    const lfs_fields lfs = { load_p, fpsimd_op_p, mem_size };
>> >  
>> > +  // Ignore the access if the register operand isn't suitable for ldp/stp.
>> > +  if (!REG_P (reg_op)
>> > +      && !SUBREG_P (reg_op)
>> > +      && (load_p || !aarch64_const_zero_rtx_p (reg_op)))
>> > +    return;
>> > +
>> 
>> It might be more natural to test this before:
>> 
>>   // We want to segregate FP/SIMD accesses from GPR accesses.
>>   //
>>   // Before RA, we use the modes, noting that stores of constant zero
>>   // operands use GPRs (even in non-integer modes).  After RA, we use
>>   // the hard register numbers.
>>   const bool fpsimd_op_p
>>     = reload_completed
>>     ? (REG_P (reg_op) && FP_REGNUM_P (REGNO (reg_op)))
>>     : (GET_MODE_CLASS (mem_mode) != MODE_INT
>>        && (load_p || !aarch64_const_zero_rtx_p (reg_op)));
>> 
>> so that that code is running with a pre-checked operand.
>
> Yeah, I agree that seems a bit more natural, I'll move the check up.
>
>> 
>> Also, how about using the predicates instead:
>> 
>>   if (load_p
>>       ? !aarch64_ldp_reg_operand (reg_op, VOIDmode)
>>       : !aarch64_stp_reg_operand (reg_op, VOIDmode))
>>     return;
>
> I thought about doing that, but it seems that we'd effectively just be
> re-doing the mode check we did above by calling ldp_operand_mode_ok_p
> (assuming generic RTL rules hold), so it seems a bit wasteful to call
> the predicates.  Given that this function is called on every (single
> set) memory access in a function, I wonder if we should prefer the
> inline check?

How about passing mem_mode to the predicates and making the
above do the mode check as well?  That feels like it would scale
well to extending forms (when implemented, and with the mode then
specifically being the mode of the SET_SRC, so that it "agrees"
with reg_op).

Richard
Alex Coplan Dec. 19, 2023, 1:52 p.m. UTC | #4 
On 19/12/2023 13:38, Richard Sandiford wrote:
> Alex Coplan <alex.coplan@arm.com> writes:
> > On 19/12/2023 10:15, Richard Sandiford wrote:
> >> Alex Coplan <alex.coplan@arm.com> writes:
> >> > We were missing validation of the candidate register operands in the
> >> > ldp/stp pass.  I was relying on recog rejecting such cases when we
> >> > formed the final pair insn, but the testcase shows that with
> >> > -fharden-conditionals we attempt to combine two insns with asm_operands,
> >> > both containing mem rtxes.  This then trips the assert:
> >> >
> >> > gcc_assert (change->new_uses.is_valid ());
> >> >
> >> > in the stp case as we aren't expecting to have (distinct) uses of mem in
> >> > the candidate stores.
> >> >
> >> > Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk?
> >> >
> >> > Thanks,
> >> > Alex
> >> >
> >> > gcc/ChangeLog:
> >> >
> >> > 	PR target/113062
> >> > 	* config/aarch64/aarch64-ldp-fusion.cc
> >> > 	(ldp_bb_info::track_access): Punt on accesses with invalid
> >> > 	register operands.
> >> >
> >> > gcc/testsuite/ChangeLog:
> >> >
> >> > 	PR target/113062
> >> > 	* gcc.dg/pr113062.c: New test.
> >> >
> >> > diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc b/gcc/config/aarch64/aarch64-ldp-fusion.cc
> >> > index 327ba4e417d..273db8c582f 100644
> >> > --- a/gcc/config/aarch64/aarch64-ldp-fusion.cc
> >> > +++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc
> >> > @@ -476,6 +476,12 @@ ldp_bb_info::track_access (insn_info *insn, bool load_p, rtx mem)
> >> >  
> >> >    const lfs_fields lfs = { load_p, fpsimd_op_p, mem_size };
> >> >  
> >> > +  // Ignore the access if the register operand isn't suitable for ldp/stp.
> >> > +  if (!REG_P (reg_op)
> >> > +      && !SUBREG_P (reg_op)
> >> > +      && (load_p || !aarch64_const_zero_rtx_p (reg_op)))
> >> > +    return;
> >> > +
> >> 
> >> It might be more natural to test this before:
> >> 
> >>   // We want to segregate FP/SIMD accesses from GPR accesses.
> >>   //
> >>   // Before RA, we use the modes, noting that stores of constant zero
> >>   // operands use GPRs (even in non-integer modes).  After RA, we use
> >>   // the hard register numbers.
> >>   const bool fpsimd_op_p
> >>     = reload_completed
> >>     ? (REG_P (reg_op) && FP_REGNUM_P (REGNO (reg_op)))
> >>     : (GET_MODE_CLASS (mem_mode) != MODE_INT
> >>        && (load_p || !aarch64_const_zero_rtx_p (reg_op)));
> >> 
> >> so that that code is running with a pre-checked operand.
> >
> > Yeah, I agree that seems a bit more natural, I'll move the check up.
> >
> >> 
> >> Also, how about using the predicates instead:
> >> 
> >>   if (load_p
> >>       ? !aarch64_ldp_reg_operand (reg_op, VOIDmode)
> >>       : !aarch64_stp_reg_operand (reg_op, VOIDmode))
> >>     return;
> >
> > I thought about doing that, but it seems that we'd effectively just be
> > re-doing the mode check we did above by calling ldp_operand_mode_ok_p
> > (assuming generic RTL rules hold), so it seems a bit wasteful to call
> > the predicates.  Given that this function is called on every (single
> > set) memory access in a function, I wonder if we should prefer the
> > inline check?
> 
> How about passing mem_mode to the predicates and making the
> above do the mode check as well?  That feels like it would scale
> well to extending forms (when implemented, and with the mode then
> specifically being the mode of the SET_SRC, so that it "agrees"
> with reg_op).

Yes, that sounds better to me, it makes the code more defensive as well
(we're actually getting some extra checking from the predicate if we do
that).

I'll respin / re-test the patch and do that.

Thanks,
Alex

> 
> Richard
diff mbox series

Patch

diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc b/gcc/config/aarch64/aarch64-ldp-fusion.cc
index 327ba4e417d..273db8c582f 100644
--- a/gcc/config/aarch64/aarch64-ldp-fusion.cc
+++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc
@@ -476,6 +476,12 @@  ldp_bb_info::track_access (insn_info *insn, bool load_p, rtx mem)
 
   const lfs_fields lfs = { load_p, fpsimd_op_p, mem_size };
 
+  // Ignore the access if the register operand isn't suitable for ldp/stp.
+  if (!REG_P (reg_op)
+      && !SUBREG_P (reg_op)
+      && (load_p || !aarch64_const_zero_rtx_p (reg_op)))
+    return;
+
   if (track_via_mem_expr (insn, mem, lfs))
     return;
 
diff --git a/gcc/testsuite/gcc.dg/pr113062.c b/gcc/testsuite/gcc.dg/pr113062.c
new file mode 100644
index 00000000000..5667c17b0f6
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr113062.c
@@ -0,0 +1,10 @@ 
+/* { dg-do compile } */
+/* { dg-options "-Oz -fharden-conditional-branches" } */
+long double foo;
+double bar;
+void abort();
+void check() {
+  if (foo == bar)
+    abort();
+}
+