Skip to content

[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

Merged
merged 2 commits into from
Jan 3, 2025

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Jan 2, 2025

This code should follow the target preference for boolean contents of a vector type. We shouldn't assume that true is negative one.

… 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.
@topperc topperc requested review from arsenm and RKSimon January 2, 2025 22:37
@llvmbot llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Jan 2, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 2, 2025

@llvm/pr-subscribers-llvm-selectiondag

Author: Craig Topper (topperc)

Changes

This 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:

  • (modified) llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp (+2-1)
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));
Copy link
Collaborator

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

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

@RKSimon RKSimon left a 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

@topperc topperc merged commit e32afde into llvm:main Jan 3, 2025
8 checks passed
@topperc topperc deleted the pr/unrollvsetcc branch January 3, 2025 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:SelectionDAG SelectionDAGISel as well
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants