-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[Fix] A replaced SDValue is used to call getNode function #82881
Conversation
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
@llvm/pr-subscribers-backend-x86 Author: RicoAfoat (RicoAfoat) ChangesSee #82431 for more infomation. Bug Trace:
// llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
void SelectionDAGISel::Select_INLINEASM(SDNode *N) {
SDLoc DL(N);
std::vector<SDValue> Ops(N->op_begin(), N->op_end());
SelectInlineAsmMemoryOperands(Ops, DL); // t39 is builded and deleted here, but it remains in Ops
const EVT VTs[] = {MVT::Other, MVT::Glue};
SDValue New = CurDAG->getNode(N->getOpcode(), DL, VTs, Ops);// t39 is a DELETED_NODE, assertion fails in builder
New->setNodeId(-1);
ReplaceUses(N, New.getNode());
CurDAG->RemoveDeadNode(N);
} // llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
SDValue X86DAGToDAGISel::matchIndexRecursively(SDValue N,
X86ISelAddressMode &AM,
unsigned Depth) {
...
if (Opc == ISD::ZERO_EXTEND && !VT.isVector() && N.hasOneUse()) {
...
if (((SrcOpc == ISD::ADD && Src->getFlags().hasNoUnsignedWrap()) ||
CurDAG->isADDLike(Src)) &&
Src.hasOneUse()) {
if (CurDAG->isBaseWithConstantOffset(Src)) {
...
if (!foldOffsetIntoAddress(Offset * AM.Scale, AM)) {
...
CurDAG->ReplaceAllUsesWith(N, ExtAdd); //t39 is replaced here, but later it will be used as
CurDAG->RemoveDeadNode(N.getNode());// parameters to call a builder method.
return Res ? Res : ExtSrc;
}
}
}
}
...
return N;
} Reason Why Modify Like This:
Full diff: https://github.com/llvm/llvm-project/pull/82881.diff 1 Files Affected:
diff --git a/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp b/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
index c8f80ced354538..50be61c56e951c 100644
--- a/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
+++ b/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
@@ -2732,13 +2732,14 @@ bool X86DAGToDAGISel::matchAddressRecursively(SDValue N, X86ISelAddressMode &AM,
insertDAGNode(*CurDAG, N, Zext);
SDValue NewShl = CurDAG->getNode(ISD::SHL, DL, VT, Zext, ShlAmt);
insertDAGNode(*CurDAG, N, NewShl);
+ CurDAG->ReplaceAllUsesWith(N, NewShl);
+ CurDAG->RemoveDeadNode(N.getNode());
// Convert the shift to scale factor.
AM.Scale = 1 << ShAmtV;
- AM.IndexReg = Zext;
-
- CurDAG->ReplaceAllUsesWith(N, NewShl);
- CurDAG->RemoveDeadNode(N.getNode());
+ // If matchIndexRecursively is not called here,
+ // Zext may be replaced by other nodes but later used to call a builder method
+ AM.IndexReg = matchIndexRecursively(Zext, AM, Depth + 1);
return false;
}
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Please can you add the #82431 test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor comments
@@ -0,0 +1,15 @@ | |||
; RUN: llc -mtriple=x86_64-unknown-linux-gnu < %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add FileCheck and run this through the update_llc_test_checks.py script to show the codegen
; calling for SelectionDAGISel::Select_INLINEASM getNode builder, see issue | ||
; 82431 for more infomation. | ||
|
||
define void @d(i8 %call, ptr %b) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(style) rename this @PR82431
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excuse me, which name should I pick : @PR82881 or mention the related issue in another style ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Historically, PR is an abbreviation for "Problem Report", so @PR82431 would be correct. See https://llvm.org/docs/TestingGuide.html#other-features
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, https://llvm.org/PR82431 opens the wrong issue.
@RKSimon I've seen other people use GHxxxxx syntax. Could it be a better choice now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR#### number should be the bug issue report, not the pull request. PR doesn't mean Pull Request it means Problem Report.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - cheers
@RicoAfoat Please can you cleanup the patch title/summary for the commit |
@RicoAfoat "[X86] Fix a replaced SDValue is used to call getNode function " doesn't explain much, maybe: "[X86] matchAddressRecursively - ensure dead nodes are replaced before matching the index register" |
…y using matchIndexRecursively Fix #82431
@RKSimon Sorry but I forgot to tell you that I don't have commit access. And buildkite start to fail on a strange flang testcase (it seems like it is an irrelevant test) although I only changed my commit message. Am I suppose to do anything else? |
@RicoAfoat I'll commit for - cheers |
@RicoAfoat Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Fixes #82431 - see #82431 for more infomation.
Bug Trace:
Zext
in the modified code piece might be replaced by otherSDValue
and deleted duringSelectInlineAsmMemoryOperand
method, but it remains as element inOps
vector which will be used to callgetNode
builder, which is illegal.Reason Why Modify Like This:
SelectInlineAsmMemoryOperands
is a method which all target machine architecture share, bug only happens inx86
, can't be fixed here.SelectInlineAsmMemoryOperand
interact withSelectInlineAsmMemoryOperand
, it is hard to modify element inOps
after it is pushed into.llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
)matchAddressRecursively
in caseISD::ZERO_EXTEND
generatesZext
andNewShl
, but the generation is not consistent with howmatchAddressRecursively
deal withISD::SHL
. The latter callsmatchIndexRecursively
to replaceZext
. I modified this so that the correctAM.IndexReg
in this situation will enterOps
.