On 19/12/2023 10:15, Richard Sandiford wrote:
> Alex Coplan <[email protected]> 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();
> > +}
> > +
