On 19/12/2023 13:38, Richard Sandiford wrote:
> Alex Coplan <alex.cop...@arm.com> writes:
> > On 19/12/2023 10:15, Richard Sandiford wrote:
> >> Alex Coplan <alex.cop...@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
