| Message ID | ZYKz2fbKlwr0hIHC@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] aarch64: Validate register operands early in ldp fusion pass [PR113062] | expand |
Alex Coplan <alex.coplan@arm.com> writes: > This is a v2 addressing Richard's feedback, v1 was posted here: > https://gcc.gnu.org/pipermail/gcc-patches/2023-December/640957.html > > Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk? > > Thanks, > Alex > > -- >8 -- > > 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. > > While doing this I noticed that it seems more natural to have the > initial definition of mem_size closer to its first use in track_access, > so I moved that down. > > gcc/ChangeLog: > > PR target/113062 > * config/aarch64/aarch64-ldp-fusion.cc > (ldp_bb_info::track_access): Punt on accesses with invalid > register operands, move definition of mem_size closer to its > first use. > > gcc/testsuite/ChangeLog: > > PR target/113062 > * gcc.dg/pr113062.c: New test. OK, thanks. Richard > diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc b/gcc/config/aarch64/aarch64-ldp-fusion.cc > index 327ba4e417d..0e2c299a0bf 100644 > --- a/gcc/config/aarch64/aarch64-ldp-fusion.cc > +++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc > @@ -458,11 +458,14 @@ ldp_bb_info::track_access (insn_info *insn, bool load_p, rtx mem) > if (!ldp_operand_mode_ok_p (mem_mode)) > return; > > - // Note ldp_operand_mode_ok_p already rejected VL modes. > - const HOST_WIDE_INT mem_size = GET_MODE_SIZE (mem_mode).to_constant (); > - > rtx reg_op = XEXP (PATTERN (insn->rtl ()), !load_p); > > + // Ignore the access if the register operand isn't suitable for ldp/stp. > + if (load_p > + ? !aarch64_ldp_reg_operand (reg_op, mem_mode) > + : !aarch64_stp_reg_operand (reg_op, mem_mode)) > + return; > + > // We want to segregate FP/SIMD accesses from GPR accesses. > // > // Before RA, we use the modes, noting that stores of constant zero > @@ -474,6 +477,8 @@ ldp_bb_info::track_access (insn_info *insn, bool load_p, rtx mem) > : (GET_MODE_CLASS (mem_mode) != MODE_INT > && (load_p || !aarch64_const_zero_rtx_p (reg_op))); > > + // Note ldp_operand_mode_ok_p already rejected VL modes. > + const HOST_WIDE_INT mem_size = GET_MODE_SIZE (mem_mode).to_constant (); > const lfs_fields lfs = { load_p, fpsimd_op_p, mem_size }; > > if (track_via_mem_expr (insn, mem, lfs)) > 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(); > +} > +
diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc b/gcc/config/aarch64/aarch64-ldp-fusion.cc index 327ba4e417d..0e2c299a0bf 100644 --- a/gcc/config/aarch64/aarch64-ldp-fusion.cc +++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc @@ -458,11 +458,14 @@ ldp_bb_info::track_access (insn_info *insn, bool load_p, rtx mem) if (!ldp_operand_mode_ok_p (mem_mode)) return; - // Note ldp_operand_mode_ok_p already rejected VL modes. - const HOST_WIDE_INT mem_size = GET_MODE_SIZE (mem_mode).to_constant (); - rtx reg_op = XEXP (PATTERN (insn->rtl ()), !load_p); + // Ignore the access if the register operand isn't suitable for ldp/stp. + if (load_p + ? !aarch64_ldp_reg_operand (reg_op, mem_mode) + : !aarch64_stp_reg_operand (reg_op, mem_mode)) + return; + // We want to segregate FP/SIMD accesses from GPR accesses. // // Before RA, we use the modes, noting that stores of constant zero @@ -474,6 +477,8 @@ ldp_bb_info::track_access (insn_info *insn, bool load_p, rtx mem) : (GET_MODE_CLASS (mem_mode) != MODE_INT && (load_p || !aarch64_const_zero_rtx_p (reg_op))); + // Note ldp_operand_mode_ok_p already rejected VL modes. + const HOST_WIDE_INT mem_size = GET_MODE_SIZE (mem_mode).to_constant (); const lfs_fields lfs = { load_p, fpsimd_op_p, mem_size }; if (track_via_mem_expr (insn, mem, lfs)) 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(); +} +
This is a v2 addressing Richard's feedback, v1 was posted here: https://gcc.gnu.org/pipermail/gcc-patches/2023-December/640957.html Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk? Thanks, Alex -- >8 -- 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. While doing this I noticed that it seems more natural to have the initial definition of mem_size closer to its first use in track_access, so I moved that down. gcc/ChangeLog: PR target/113062 * config/aarch64/aarch64-ldp-fusion.cc (ldp_bb_info::track_access): Punt on accesses with invalid register operands, move definition of mem_size closer to its first use. gcc/testsuite/ChangeLog: PR target/113062 * gcc.dg/pr113062.c: New test.