-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[LegalizeVectorOps] Use getBoolConstant instead of getAllOnesConstant in VectorLegalizer::UnrollVSETCC. #121526
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
Conversation
… in VectorLegalizer::UnrollVSETCC. This code should follow the target preference for boolean contents of a vector type. We shouldn't assume that true is negative one.
@llvm/pr-subscribers-llvm-selectiondag Author: Craig Topper (topperc) ChangesThis code should follow the target preference for boolean contents of a vector type. We shouldn't assume that true is negative one. Full diff: https://github.com/llvm/llvm-project/pull/121526.diff 1 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
index db21e708970648..93e081ec9ccc02 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
@@ -2250,7 +2250,8 @@ SDValue VectorLegalizer::UnrollVSETCC(SDNode *Node) {
TLI.getSetCCResultType(DAG.getDataLayout(),
*DAG.getContext(), TmpEltVT),
LHSElem, RHSElem, CC);
- Ops[i] = DAG.getSelect(dl, EltVT, Ops[i], DAG.getAllOnesConstant(dl, EltVT),
+ Ops[i] = DAG.getSelect(dl, EltVT, Ops[i],
+ DAG.getBoolConstant(true, dl, EltVT, VT),
DAG.getConstant(0, dl, EltVT));
}
return DAG.getBuildVector(VT, dl, Ops);
|
@@ -2250,7 +2250,8 @@ SDValue VectorLegalizer::UnrollVSETCC(SDNode *Node) { | |||
TLI.getSetCCResultType(DAG.getDataLayout(), | |||
*DAG.getContext(), TmpEltVT), | |||
LHSElem, RHSElem, CC); | |||
Ops[i] = DAG.getSelect(dl, EltVT, Ops[i], DAG.getAllOnesConstant(dl, EltVT), | |||
Ops[i] = DAG.getSelect(dl, EltVT, Ops[i], | |||
DAG.getBoolConstant(true, dl, EltVT, VT), | |||
DAG.getConstant(0, dl, EltVT)); |
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.
I still think this should be:
Ops[i] = DAG.getNode(ISD::SETCC, dl, MVT::i1, LHSElem, RHSElem, CC);
Ops[i] = DAG.getBoolExtOrTrunc(Ops[i], dl, EltVT, VT);
But the last time I attempted this it resulted in a lot of churn: https://github.com/RKSimon/llvm-project/tree/unroll-setcc
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.
Thanks. I didn't know there was history here. Have any of the yaks been shaved yet?
Still seems like we should either fix the code with getBoolConstant or add a FIXME until we can use getBoolExtOrTrunc. That way the next person who reads this code doesn't think it's wrong like I just did.
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.
Agreed, a fixme comment would be acceptable for 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.
LGTM with a suitable comment that we should be avoiding the select and performing a setcc+boolext instead
This code should follow the target preference for boolean contents of a vector type. We shouldn't assume that true is negative one.