On 29/01/2025 18:46, Richard Sandiford wrote:
> As Andrew says in the bugzilla comments, this PR is about a case where
> we tried to fuse two stores of x0, one in which x0 was defined and one
> in which it was undefined. merge_access_arrays failed on the conflict,
> but the failure wasn't caught.
>
> Normally the hazard detection code would fail if the instructions
> had incompatible uses. However, an undefined use doesn't impose
> many restrictions on movements. I think this is likely to be the
> only case where hazard detection isn't enough.
>
> As Andrew notes in bugzilla, it might be possible to allow uses
> of defined and undefined values to be merged to the defined value.
> But that sounds dangerous in the general case, as an rtl-ssa-level
> decision. We might run the risk of turning conditional UB into
> unconditional UB. And LLVM proves that the definition of "undef"
> isn't simple.
>
> Tested on aarch64-linux-gnu. OK to install?
Thanks for taking care of this. LGTM, but I have a question below, just
for my own understanding ...
>
> Richard
>
>
> gcc/
> PR rtl-optimization/118320
> * pair-fusion.cc (pair_fusion_bb_info::fuse_pair): Commonize
> the merge of input_uses and return early if it fails.
>
> gcc/testsuite/
> PR rtl-optimization/118320
> * g++.dg/torture/pr118320.C: New test.
> ---
> gcc/pair-fusion.cc | 32 ++++++++++++++++---------
> gcc/testsuite/g++.dg/torture/pr118320.C | 15 ++++++++++++
> 2 files changed, 36 insertions(+), 11 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/torture/pr118320.C
>
> diff --git a/gcc/pair-fusion.cc b/gcc/pair-fusion.cc
> index 602e572ab6c..5708d0f3b67 100644
> --- a/gcc/pair-fusion.cc
> +++ b/gcc/pair-fusion.cc
> @@ -1730,6 +1730,24 @@ pair_fusion_bb_info::fuse_pair (bool load_p,
> input_uses[i] = remove_note_accesses (attempt, input_uses[i]);
> }
>
> + // Get the uses that the pair instruction would have, and punt if
> + // the unpaired instructions use different definitions of the same
> + // register. That would normally be caught as a side-effect of
> + // hazard detection below, but this check also deals with cases
> + // in which one use is undefined and the other isn't.
> + auto new_uses = merge_access_arrays (attempt,
> + drop_memory_access (input_uses[0]),
> + drop_memory_access (input_uses[1]));
> + if (!new_uses.is_valid ())
> + {
> + if (dump_file)
> + fprintf (dump_file,
> + " load pair: i%d and i%d use different definiitions of"
... how do we know that this is a load pair here? Could this not in
theory trigger for stores too?
Thanks,
Alex
> + " the same register\n",
> + insns[0]->uid (), insns[1]->uid ());
> + return false;
> + }
> +
> // Edge case: if the first insn is a writeback load and the
> // second insn is a non-writeback load which transfers into the base
> // register, then we should drop the writeback altogether as the
> @@ -1852,11 +1870,7 @@ pair_fusion_bb_info::fuse_pair (bool load_p,
> input_defs[1]);
> gcc_assert (pair_change->new_defs.is_valid ());
>
> - pair_change->new_uses
> - = merge_access_arrays (attempt,
> - drop_memory_access (input_uses[0]),
> - drop_memory_access (input_uses[1]));
> - gcc_assert (pair_change->new_uses.is_valid ());
> + pair_change->new_uses = new_uses;
> set_pair_pat (pair_change);
> }
> else
> @@ -1877,9 +1891,7 @@ pair_fusion_bb_info::fuse_pair (bool load_p,
> case Action::CHANGE:
> {
> set_pair_pat (change);
> - change->new_uses = merge_access_arrays (attempt,
> - input_uses[0],
> - input_uses[1]);
> + change->new_uses = new_uses;
> auto d1 = drop_memory_access (input_defs[0]);
> auto d2 = drop_memory_access (input_defs[1]);
> change->new_defs = merge_access_arrays (attempt, d1, d2);
> @@ -1907,9 +1919,7 @@ pair_fusion_bb_info::fuse_pair (bool load_p,
> auto new_insn = crtl->ssa->create_insn (attempt, INSN, pair_pat);
> change = make_change (new_insn);
> change->move_range = move_range;
> - change->new_uses = merge_access_arrays (attempt,
> - input_uses[0],
> - input_uses[1]);
> + change->new_uses = new_uses;
> gcc_assert (change->new_uses.is_valid ());
>
> auto d1 = drop_memory_access (input_defs[0]);
> diff --git a/gcc/testsuite/g++.dg/torture/pr118320.C
> b/gcc/testsuite/g++.dg/torture/pr118320.C
> new file mode 100644
> index 00000000000..228d79860b5
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/torture/pr118320.C
> @@ -0,0 +1,15 @@
> +// { dg-additional-options "-fno-tree-sra" } */
> +
> +struct T{
> + long f[2];
> +};
> +void f1(int);
> +void g(T&);
> +void f1()
> +{
> + T a;
> + a.~T(); // To force the clobber
> + a.f[1] = 1;
> + T b = a;
> + g(b);
> +}
> --
> 2.25.1
>
