[SelectionDAG][X86] Fix the assertion failure in Release build after #91747 #93434
Conversation
…lvm#91747 In llvm#91747, we changed the SDNode from `X86ISD::SUB` (FROM) to `X86ISD::CCMP` (TO) in the DAGCombine. The value type of `X86ISD::SUB` can be `i8, i32` while the value type of `X86ISD::CCMP` is i32. This breaks the assumption that the value type should match after the combine and triggers the error ``` SelectionDAG.cpp:10942: void llvm::SelectionDAG::transferDbgValues(llvm::SDValue, llvm::SDValue, unsigned int, unsigned int, bool): Assertion `FromNode && ToNode && "Can't modify dbg values"' failed. ``` when running tests llvm/test/CodeGen/X86/apx/ccmp.ll llvm/test/CodeGen/X86/apx/ctest.ll in Release build when LLVM_ENABLE_ASSERTIONS is on. In this patch, we fix it by creating a merged value.
|
@llvm/pr-subscribers-llvm-selectiondag @llvm/pr-subscribers-backend-x86 Author: Shengchen Kan (KanRobert) ChangesIn #91747, we changed the SDNode from when running tests llvm/test/CodeGen/X86/apx/ccmp.ll in Release build when LLVM_ENABLE_ASSERTIONS is on. In this patch, we fix it by creating a merged value. Full diff: https://github.com/llvm/llvm-project/pull/93434.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index c7aeb0633e4ba..93d866384b482 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -1801,8 +1801,11 @@ void DAGCombiner::Run(CombineLevel AtLevel) {
if (N->getNumValues() == RV->getNumValues())
DAG.ReplaceAllUsesWith(N, RV.getNode());
- else
+ else {
+ assert(N->getValueType(0) == RV.getValueType() &&
+ N->getNumValues() == 1 && "Type mismatch");
DAG.ReplaceAllUsesWith(N, &RV);
+ }
// Push the new node and any users onto the worklist. Omit this if the
// new node is the EntryToken (e.g. if a store managed to get optimized
diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp
index 7d90296a3eea6..09284839b9680 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -49253,8 +49253,9 @@ static SDValue combineX86SubCmpForFlags(SDNode *N, SDValue Flag,
return SDValue();
SDValue X = SetCC.getOperand(1);
- // Replace API is called manually here b/c the number of results may change.
- DAG.ReplaceAllUsesOfValueWith(Flag, X);
+ // sub has two results while X only have one. DAG combine assumes the value type match.
+ if (N->getNumValues() > 1)
+ X = DAG.getMergeValues({N->getOperand(0), X}, SDLoc(N));
SDValue CCN = SetCC.getOperand(0);
X86::CondCode CC =
|
|
Thanks a lot for the suggestion @phoebewang ! |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| DAG.ReplaceAllUsesOfValueWith(Flag, X); | ||
| // sub has two results while X only have one. DAG combine assumes the value | ||
| // type matches. | ||
| if (N->getNumValues() > 1) |
There was a problem hiding this comment.
Doesn't N always have 2 results?
There was a problem hiding this comment.
N can be a X86ISD::CMP or X86ISD::SUB. When N is CMP, it only has one result.
There was a problem hiding this comment.
How about if (N->getOpcode() == X86ISD::SUB) for clarity?
There was a problem hiding this comment.
Good suggestion, done
| DAG.ReplaceAllUsesOfValueWith(Flag, X); | ||
| // sub has two results while X only have one. DAG combine assumes the value | ||
| // type matches. | ||
| if (N->getNumValues() > 1) |
There was a problem hiding this comment.
How about if (N->getOpcode() == X86ISD::SUB) for clarity?
|
I will merge it soon to prevent this failure from causing trouble to other developers |
In #91747, we changed the SDNode from
X86ISD::SUB(FROM) toX86ISD::CCMP(TO) in the DAGCombine. The value type of
X86ISD::SUBcan bei8, i32while the value type of
X86ISD::CCMPis i32. This breaks the assumptionthat the value type should match after the combine and triggers the
error
when running tests
llvm/test/CodeGen/X86/apx/ccmp.ll
llvm/test/CodeGen/X86/apx/ctest.ll
in Release build when LLVM_ENABLE_ASSERTIONS is on.
In this patch, we fix it by creating a merged value.