* [PATCH v2] aarch64: Validate register operands early in ldp fusion pass [PR113062]
@ 2023-12-20 9:28 Alex Coplan
2023-12-20 9:34 ` Richard Sandiford
0 siblings, 1 reply; 2+ messages in thread
From: Alex Coplan @ 2023-12-20 9:28 UTC (permalink / raw)
To: gcc-patches; +Cc: Richard Sandiford, Kyrylo Tkachov
[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]
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.
[-- Attachment #2: patch_v2.txt --]
[-- Type: text/plain, Size: 1740 bytes --]
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();
+}
+
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH v2] aarch64: Validate register operands early in ldp fusion pass [PR113062]
2023-12-20 9:28 [PATCH v2] aarch64: Validate register operands early in ldp fusion pass [PR113062] Alex Coplan
@ 2023-12-20 9:34 ` Richard Sandiford
0 siblings, 0 replies; 2+ messages in thread
From: Richard Sandiford @ 2023-12-20 9:34 UTC (permalink / raw)
To: Alex Coplan; +Cc: gcc-patches, Kyrylo Tkachov
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();
> +}
> +
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-12-20 9:34 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-20 9:28 [PATCH v2] aarch64: Validate register operands early in ldp fusion pass [PR113062] Alex Coplan
2023-12-20 9:34 ` Richard Sandiford
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).