Alex Coplan <alex.cop...@arm.com> writes:
> 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?
You're right, and as mentioned above, stores were the motivating case.
I'd just copied this style mechanically from a neighbouring dump message,
on the assumption that "load pair" was being used a generic term,
but I see now that it isn't.
So yeah, please feel free to correct to whatever you think is better.
(The patch is already committed, since there was a request to get this
fixed while you were away.)
Thanks,
Richard
