-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[SelectionDAG][RISCV] Avoid store merging across function calls #130430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 12 commits
3e7e708
5b1fc65
be40ec2
c3298ff
b56d1b1
82420c7
96c8e53
3574370
f9393d5
d86ec01
04bca6d
f27092f
9faa629
b326da1
c858020
b6b1521
18e68ea
3bc2b22
816a235
904641f
a255f16
75f4caa
de96633
69c361c
0dfd354
e73c49d
a88e73b
d6c848d
0189f30
99e11ae
67b3b65
ed8a5fd
21ba7b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21553,6 +21553,56 @@ bool DAGCombiner::tryStoreMergeOfLoads(SmallVectorImpl<MemOpLink> &StoreNodes, | |
| JointMemOpVT = EVT::getIntegerVT(Context, SizeInBits); | ||
| } | ||
|
|
||
| auto HasCallInLdStChain = [](SmallVectorImpl<MemOpLink> &StoreNodes, | ||
mikhailramalho marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| SmallVectorImpl<MemOpLink> &LoadNodes, | ||
| unsigned NumStores) { | ||
| for (unsigned i = 0; i < NumStores; ++i) { | ||
| StoreSDNode *St = cast<StoreSDNode>(StoreNodes[i].MemNode); | ||
| SDValue Val = peekThroughBitcasts(St->getValue()); | ||
| LoadSDNode *Ld = cast<LoadSDNode>(Val); | ||
| assert(Ld == LoadNodes[i].MemNode && "Load and store mismatch"); | ||
mikhailramalho marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| SmallPtrSet<const SDNode *, 32> Visited; | ||
| SmallVector<std::pair<const SDNode *, bool>, 8> Worklist; | ||
| Worklist.emplace_back(St->getOperand(0).getNode(), false); | ||
mikhailramalho marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| while (!Worklist.empty()) { | ||
| auto [Node, FoundCall] = Worklist.pop_back_val(); | ||
| if (!Visited.insert(Node).second || Node->getNumOperands() == 0) | ||
| continue; | ||
|
|
||
| switch (Node->getOpcode()) { | ||
| case ISD::CALLSEQ_END: | ||
| Worklist.emplace_back(Node->getOperand(0).getNode(), true); | ||
| break; | ||
| case ISD::TokenFactor: | ||
| for (SDValue Op : Node->ops()) | ||
| Worklist.emplace_back(Op.getNode(), FoundCall); | ||
| break; | ||
| case ISD::LOAD: | ||
| if (Node == Ld) | ||
| return FoundCall; | ||
| [[fallthrough]]; | ||
| default: | ||
| if (Node->getNumOperands() > 0) | ||
mikhailramalho marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| Worklist.emplace_back(Node->getOperand(0).getNode(), FoundCall); | ||
mikhailramalho marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| break; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
| return false; | ||
| }; | ||
|
|
||
| // Check if there is a call in the load/store chain. | ||
| if (!TLI.shouldMergeStoreOfLoadsOverCall(JointMemOpVT) && | ||
| HasCallInLdStChain(StoreNodes, LoadNodes, NumElem)) { | ||
| StoreNodes.erase(StoreNodes.begin(), StoreNodes.begin() + NumElem); | ||
| LoadNodes.erase(LoadNodes.begin(), LoadNodes.begin() + NumElem); | ||
| NumConsecutiveStores -= NumElem; | ||
| continue; | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just noticed there is a better integration here. The way you have this phrased, we either can merge to the vector type, or we don't merge at all. I think what you actually want to do here is to add a couple parameters to the existing canMergeStoresTo interface. If you add both the SrcVT, and a "IsOverCall" boolean, then that interface returning the answer suggested above in the IsOverCall case will result in a wider scalar type being chosen if one is available.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though, I'll note I'm also fine in this being a follow up change. I'm happy to pick that up if you want to focus on just getting this in.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm happy with either option: to land this PR and do a follow-up patch, or work on this one. Up to you. Also, I did a couple of tests with your first suggestion on checking for CSR for a given type in mikhailramalho@df27cb2, should I give up on that approach? There are still a couple of crashes I need to debug.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's go for minimum viable patch. Update the existing TLI hook you added with the source parameter, let's get that landed, and then iterate. |
||
|
|
||
| SDLoc LoadDL(LoadNodes[0].MemNode); | ||
| SDLoc StoreDL(StoreNodes[0].MemNode); | ||
|
|
||
|
|
||
mikhailramalho marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
Uh oh!
There was an error while loading. Please reload this page.