[RISCV] Reorganize select lowering to pull binop expansion early #156974
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is purely stylistic, but I think makes the code easier to follow. It isn't quite NFC because it undoes the airthmetic lowering for the select c, simm12, 0 cases for a processor with both conditional move forwarding and zicond.
@topperc It's not really clear what I should do for the configuration mentioned above. Do you actually have such a processor? If so, how do you want the codegen to look for this case? |
|
@llvm/pr-subscribers-backend-risc-v Author: Philip Reames (preames) ChangesThis is purely stylistic, but I think makes the code easier to follow. It isn't quite NFC because it undoes the arithmetic lowering for the select c, simm12, 0 cases for a processor with both conditional move forwarding and zicond. Full diff: https://github.com/llvm/llvm-project/pull/156974.diff 2 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index f8ec1be1fd8d6..d26f5ab8f3e88 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -9106,8 +9106,12 @@ static std::optional<bool> matchSetCC(SDValue LHS, SDValue RHS,
return std::nullopt;
}
-static SDValue combineSelectToBinOp(SDNode *N, SelectionDAG &DAG,
- const RISCVSubtarget &Subtarget) {
+static bool isSimm12Constant(SDValue V) {
+ return isa<ConstantSDNode>(V) && V->getAsAPIntVal().isSignedIntN(12);
+}
+
+static SDValue lowerSelectToBinOp(SDNode *N, SelectionDAG &DAG,
+ const RISCVSubtarget &Subtarget) {
SDValue CondV = N->getOperand(0);
SDValue TrueV = N->getOperand(1);
SDValue FalseV = N->getOperand(2);
@@ -9127,14 +9131,18 @@ static SDValue combineSelectToBinOp(SDNode *N, SelectionDAG &DAG,
return DAG.getNode(ISD::OR, DL, VT, Neg, DAG.getFreeze(TrueV));
}
+ const bool HasCZero =
+ VT.isScalarInteger() &&
+ (Subtarget.hasStdExtZicond() || Subtarget.hasVendorXVentanaCondOps());
+
// (select c, 0, y) -> (c-1) & y
- if (isNullConstant(TrueV)) {
- SDValue Neg = DAG.getNode(ISD::ADD, DL, VT, CondV,
- DAG.getAllOnesConstant(DL, VT));
+ if (isNullConstant(TrueV) && (!HasCZero || isSimm12Constant(FalseV))) {
+ SDValue Neg =
+ DAG.getNode(ISD::ADD, DL, VT, CondV, DAG.getAllOnesConstant(DL, VT));
return DAG.getNode(ISD::AND, DL, VT, Neg, DAG.getFreeze(FalseV));
}
// (select c, y, 0) -> -c & y
- if (isNullConstant(FalseV)) {
+ if (isNullConstant(FalseV) && (!HasCZero || isSimm12Constant(TrueV))) {
SDValue Neg = DAG.getNegative(CondV, DL, VT);
return DAG.getNode(ISD::AND, DL, VT, Neg, DAG.getFreeze(TrueV));
}
@@ -9240,10 +9248,6 @@ foldBinOpIntoSelectIfProfitable(SDNode *BO, SelectionDAG &DAG,
return DAG.getSelect(DL, VT, Sel.getOperand(0), NewT, NewF);
}
-static bool isSimm12Constant(SDValue V) {
- return isa<ConstantSDNode>(V) && V->getAsAPIntVal().isSignedIntN(12);
-}
-
SDValue RISCVTargetLowering::lowerSELECT(SDValue Op, SelectionDAG &DAG) const {
SDValue CondV = Op.getOperand(0);
SDValue TrueV = Op.getOperand(1);
@@ -9259,6 +9263,10 @@ SDValue RISCVTargetLowering::lowerSELECT(SDValue Op, SelectionDAG &DAG) const {
return DAG.getNode(ISD::VSELECT, DL, VT, CondSplat, TrueV, FalseV);
}
+ // Try some other optimizations before falling back to generic lowering.
+ if (SDValue V = lowerSelectToBinOp(Op.getNode(), DAG, Subtarget))
+ return V;
+
// When Zicond or XVentanaCondOps is present, emit CZERO_EQZ and CZERO_NEZ
// nodes to implement the SELECT. Performing the lowering here allows for
// greater control over when CZERO_{EQZ/NEZ} are used vs another branchless
@@ -9266,19 +9274,6 @@ SDValue RISCVTargetLowering::lowerSELECT(SDValue Op, SelectionDAG &DAG) const {
if ((Subtarget.hasStdExtZicond() || Subtarget.hasVendorXVentanaCondOps()) &&
VT.isScalarInteger()) {
- // select c, simm12, 0 -> andi (sub x0, c), simm12
- if (isSimm12Constant(TrueV) && isNullConstant(FalseV)) {
- SDValue Mask = DAG.getNegative(CondV, DL, VT);
- return DAG.getNode(ISD::AND, DL, VT, TrueV, Mask);
- }
-
- // select c, 0, simm12 -> andi (addi c, -1), simm12
- if (isNullConstant(TrueV) && isSimm12Constant(FalseV)) {
- SDValue Mask = DAG.getNode(ISD::ADD, DL, VT, CondV,
- DAG.getSignedConstant(-1, DL, XLenVT));
- return DAG.getNode(ISD::AND, DL, VT, FalseV, Mask);
- }
-
// (select c, t, 0) -> (czero_eqz t, c)
if (isNullConstant(FalseV))
return DAG.getNode(RISCVISD::CZERO_EQZ, DL, VT, TrueV, CondV);
@@ -9332,10 +9327,6 @@ SDValue RISCVTargetLowering::lowerSELECT(SDValue Op, SelectionDAG &DAG) const {
DAG.getNode(RISCVISD::CZERO_EQZ, DL, VT, TrueV, CondV));
}
- // Try some other optimizations before falling back to generic lowering.
- if (SDValue V = combineSelectToBinOp(Op.getNode(), DAG, Subtarget))
- return V;
-
// (select c, c1, c2) -> (add (czero_nez c2 - c1, c), c1)
// (select c, c1, c2) -> (add (czero_eqz c1 - c2, c), c2)
if (isa<ConstantSDNode>(TrueV) && isa<ConstantSDNode>(FalseV)) {
@@ -9438,9 +9429,6 @@ SDValue RISCVTargetLowering::lowerSELECT(SDValue Op, SelectionDAG &DAG) const {
SDNodeFlags::Disjoint);
}
- if (SDValue V = combineSelectToBinOp(Op.getNode(), DAG, Subtarget))
- return V;
-
if (Op.hasOneUse()) {
unsigned UseOpc = Op->user_begin()->getOpcode();
if (isBinOp(UseOpc) && DAG.isSafeToSpeculativelyExecute(UseOpc)) {
diff --git a/llvm/test/CodeGen/RISCV/cmov-branch-opt.ll b/llvm/test/CodeGen/RISCV/cmov-branch-opt.ll
index 351b02494ae85..6608874286e34 100644
--- a/llvm/test/CodeGen/RISCV/cmov-branch-opt.ll
+++ b/llvm/test/CodeGen/RISCV/cmov-branch-opt.ll
@@ -149,9 +149,8 @@ define signext i32 @test4(i32 signext %x, i32 signext %y, i32 signext %z) {
;
; CMOV-ZICOND-LABEL: test4:
; CMOV-ZICOND: # %bb.0:
-; CMOV-ZICOND-NEXT: snez a0, a2
-; CMOV-ZICOND-NEXT: addi a0, a0, -1
-; CMOV-ZICOND-NEXT: andi a0, a0, 3
+; CMOV-ZICOND-NEXT: li a0, 3
+; CMOV-ZICOND-NEXT: czero.nez a0, a0, a2
; CMOV-ZICOND-NEXT: ret
;
; SFB-NOZICOND-LABEL: test4:
@@ -165,9 +164,8 @@ define signext i32 @test4(i32 signext %x, i32 signext %y, i32 signext %z) {
;
; SFB-ZICOND-LABEL: test4:
; SFB-ZICOND: # %bb.0:
-; SFB-ZICOND-NEXT: snez a0, a2
-; SFB-ZICOND-NEXT: addi a0, a0, -1
-; SFB-ZICOND-NEXT: andi a0, a0, 3
+; SFB-ZICOND-NEXT: li a0, 3
+; SFB-ZICOND-NEXT: czero.nez a0, a0, a2
; SFB-ZICOND-NEXT: ret
%c = icmp eq i32 %z, 0
%a = select i1 %c, i32 3, i32 0
|
To make sure I understand, this is the arithmetic lowering that was added in today in #156957? So we go back to what we had before today? |
Yep, exactly. |
I'll take a note to investigate this on our side. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/181/builds/27384 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/116/builds/17977 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is purely stylistic, but I think makes the code easier to follow.
It isn't quite NFC because it undoes the arithmetic lowering for the select c, simm12, 0 cases for a processor with both conditional move forwarding and zicond.