This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] don't reuse the pointer info for merged store
ClosedPublic

Authored by shchenz on Feb 24 2023, 1:45 AM.

Details

Summary

The merged store touches memory for other underlying objects, so mapping
the merged store to the first underlying object is not correct. For example
in https://github.com/llvm/llvm-project/issues/60744, the merged store is
not correctly analyzed as dependent with memory operations which are also
part of the merged store.

Fix https://github.com/llvm/llvm-project/issues/60744

Diff Detail

Event Timeline

shchenz created this revision.Feb 24 2023, 1:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2023, 1:45 AM
shchenz requested review of this revision.Feb 24 2023, 1:45 AM
foad added inline comments.Feb 24 2023, 2:02 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
19168

I agree that this looks conservatively safe, but I think it will pessimize the case where the MachinePointerInfos had the same underlying pointer.

pengfei added inline comments.Feb 25 2023, 5:42 AM
llvm/test/CodeGen/X86/merge-store-dependency.ll
14

CHECKT -> CHECK

shchenz updated this revision to Diff 500649.Feb 26 2023, 7:44 PM

still reuse the first store pointer info for cases where narrow stores all point to the same underlying object

shchenz marked 2 inline comments as done.Feb 26 2023, 7:45 PM
foad added inline comments.Feb 27 2023, 6:35 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
729

This should just take one ArrayRef<MemOpLink> argument.

18987

You don't need a set for this. You only need to remember a single Value *.

shchenz updated this revision to Diff 500997.Feb 27 2023, 6:24 PM
shchenz marked 2 inline comments as done.

address @foad comments

foad added inline comments.Feb 28 2023, 2:20 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
730

You should not need NumStores. ArrayRef knows its own length.

shchenz updated this revision to Diff 501061.Feb 28 2023, 2:37 AM

remove the unnecessary parameter.

nikic added a subscriber: nikic.Feb 28 2023, 2:40 AM
nikic added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
18995

This should use getUnderlyingObject() unless there's strong reason to be aggressive here.

shchenz updated this revision to Diff 501070.Feb 28 2023, 3:19 AM
shchenz marked 2 inline comments as done.

use getUnderlyingObject()

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
18995

Thanks. Looks good to me. No handling for multiple objects even use getUnderlyingObjects() to reduce the complexity.

gentle ping

foad added inline comments.Mar 7 2023, 2:41 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
18984

Remove NumStores and use range-based for loops like for (const auto &StoreNode : StoreNodes). Maybe also combine the two loops into one?

shchenz updated this revision to Diff 502974.Mar 7 2023, 3:19 AM
shchenz marked an inline comment as done.

address @foad comments

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
18984

Make senses Thanks for the good lessons.

foad accepted this revision.Mar 7 2023, 4:20 AM

LGTM.

This revision is now accepted and ready to land.Mar 7 2023, 4:20 AM
This revision was landed with ongoing or failed builds.Mar 7 2023, 5:32 PM
This revision was automatically updated to reflect the committed changes.
uabelho added a subscriber: uabelho.Mar 8 2023, 3:48 AM

Hi,

I'm seeing a miscompile for my out-of-tree target with this patch. I don't have a reproducer I can share and I'm not sure what is happening yet.
It might very well be something we have broken downstream, but I want to give a heads-up in case other people see problems too.

Hi,

I'm seeing a miscompile for my out-of-tree target with this patch. I don't have a reproducer I can share and I'm not sure what is happening yet.
It might very well be something we have broken downstream, but I want to give a heads-up in case other people see problems too.

And now I think it's indeed a downstream bug simply exposed by this patch.
Sorry for the noise!