[SelectionDAGISel] Don't merge input chains if it would put a token factor in the way of a glue. #167805
Conversation
…actor in the way of a glue. In the new test, we're trying to fold a load and a X86ISD::CALL. The call has a CopyToReg glued to it. The load and the call have different input chains so they need to be merged. This results in a TokenFactor that gets put between the CopyToReg and the final CALLm instruction. The DAG scheduler can't handle that. The load here was created by legalization of the extract_element using a stack temporary store and load. A normal IR load would be chained into call sequence by SelectionDAGBuilder. This would usually have the load chained in before the CopyToReg. The store/load created by legalization don't get chained into the rest of the DAG. Fixes llvm#63790
|
@llvm/pr-subscribers-llvm-selectiondag @llvm/pr-subscribers-backend-x86 Author: Craig Topper (topperc) ChangesIn the new test, we're trying to fold a load and a X86ISD::CALL. The call has a CopyToReg glued to it. The load and the call have different input chains so they need to be merged. This results in a TokenFactor that gets put between the CopyToReg and the final CALLm instruction. The DAG scheduler can't handle that. The load here was created by legalization of the extract_element using a stack temporary store and load. A normal IR load would be chained into call sequence by SelectionDAGBuilder. This would usually have the load chained in before the CopyToReg. The store/load created by legalization don't get chained into the rest of the DAG. Fixes #63790 Full diff: https://github.com/llvm/llvm-project/pull/167805.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
index e78dfb12505c7..9aac0ff3cd8a7 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
@@ -2783,7 +2783,7 @@ void SelectionDAGISel::UpdateChains(
/// be used as the input node chain for the generated nodes.
static SDValue
HandleMergeInputChains(SmallVectorImpl<SDNode*> &ChainNodesMatched,
- SelectionDAG *CurDAG) {
+ SDValue InputGlue, SelectionDAG *CurDAG) {
SmallPtrSet<const SDNode *, 16> Visited;
SmallVector<const SDNode *, 8> Worklist;
@@ -2826,8 +2826,16 @@ HandleMergeInputChains(SmallVectorImpl<SDNode*> &ChainNodesMatched,
// node that is both the predecessor and successor of the
// to-be-merged nodes. Fail.
Visited.clear();
- for (SDValue V : InputChains)
+ for (SDValue V : InputChains) {
+ // If we need to create a TokenFactor, and any of the input chains will
+ // also be glued to the output, we cannot merge the chains. The TokenFactor
+ // would prevent the glue from being honored.
+ if (InputChains.size() != 1 &&
+ V->getValueType(V->getNumValues()-1) == MVT::Glue &&
+ InputGlue.getNode() == V.getNode())
+ return SDValue();
Worklist.push_back(V.getNode());
+ }
for (auto *N : ChainNodesMatched)
if (SDNode::hasPredecessorHelper(N, Visited, Worklist, Max, true))
@@ -3989,7 +3997,7 @@ void SelectionDAGISel::SelectCodeCommon(SDNode *NodeToMatch,
}
// Merge the input chains if they are not intra-pattern references.
- InputChain = HandleMergeInputChains(ChainNodesMatched, CurDAG);
+ InputChain = HandleMergeInputChains(ChainNodesMatched, InputGlue, CurDAG);
if (!InputChain.getNode())
break; // Failed to merge.
@@ -4033,7 +4041,7 @@ void SelectionDAGISel::SelectCodeCommon(SDNode *NodeToMatch,
break;
// Merge the input chains if they are not intra-pattern references.
- InputChain = HandleMergeInputChains(ChainNodesMatched, CurDAG);
+ InputChain = HandleMergeInputChains(ChainNodesMatched, InputGlue, CurDAG);
if (!InputChain.getNode())
break; // Failed to merge.
diff --git a/llvm/test/CodeGen/X86/pr63790.ll b/llvm/test/CodeGen/X86/pr63790.ll
new file mode 100644
index 0000000000000..608d3ff41d164
--- /dev/null
+++ b/llvm/test/CodeGen/X86/pr63790.ll
@@ -0,0 +1,10 @@
+; RUN: llc < %s -mtriple=x86_64 | FileCheck %s
+
+define void @f(ptr %0, i64 %1) {
+BB:
+ %fps = load <2 x ptr>, ptr %0
+ %fp = extractelement <2 x ptr> %fps, i64 %1
+ %p = call ptr %fp(i32 42)
+ store <2 x ptr> %fps, ptr %p
+ ret void
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
s-barannikov
left a comment
s-barannikov
left a comment
There was a problem hiding this comment.
Can this be fixed by adjusting SelectionDAGISel::IsLegalToFold ? It seems to have a check for something similar.
IsLegalToFold is only called for the load node. The Glue and other chain are on the call. We could have IsLegalToFold do additional checks on the root node by checking that it has a chain operand. But in theory the other chain could be another node that isn't the root. |
3 participants
In the new test, we're trying to fold a load and a X86ISD::CALL. The call has a CopyToReg glued to it. The load and the call have different input chains so they need to be merged. This results in a TokenFactor that gets put between the CopyToReg and the final CALLm instruction. The DAG scheduler can't handle that.
The load here was created by legalization of the extract_element using a stack temporary store and load. A normal IR load would be chained into call sequence by SelectionDAGBuilder. This would usually have the load chained in before the CopyToReg. The store/load created by legalization don't get chained into the rest of the DAG.
Fixes #63790