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).
Richard
