Skip to content
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

[RISCV] Break the (czero_eqz x, (setne x, 0)) -> x combine into 2 combines. #90428

Merged
merged 2 commits into from
Apr 29, 2024

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Apr 29, 2024

We can think of this as two separate combines

(czero_eqz x, (setne y, 0)) -> (czero_eqz x, y)
and
(czero_eqz x, x) -> x

Similary the (czero_nez x, (seteq x, 0)) -> x combine can be broken into

(czero_nez x, (seteq y, 0)) -> (czero_eqz x, y)
and
(czero_eqz x, x) -> x

isel already does the (czero_eqz x, (setne y, 0)) -> (czero_eqz x, y) and (czero_nez x, (seteq y, 0)) -> (czero_eqz x, y) combines, but doing them early could expose other opportunities.

…bines.

We can think of this as two separate combines

(czero_eqz x, x) -> x
and
(czero_eqz x, (setne y, 0)) -> (czero_eqz x, y)

Similary the (czero_nez x, (seteq x, 0)) -> x combine can be
broken into

(czero_eqz x, x) -> x
and
(czero_nez x, (seteq y, 0)) -> (czero_eqz x, y)

isel already does the (czero_eqz x, (setne y, 0)) -> (czero_eqz x, y)
and (czero_nez x, (seteq y, 0)) -> (czero_eqz x, y) combines, but
doing them early could expose other opportunities.
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 29, 2024

@llvm/pr-subscribers-backend-risc-v

Author: Craig Topper (topperc)

Changes

We can think of this as two separate combines

(czero_eqz x, (setne y, 0)) -> (czero_eqz x, y)
and
(czero_eqz x, x) -> x

Similary the (czero_nez x, (seteq x, 0)) -> x combine can be broken into

(czero_nez x, (seteq y, 0)) -> (czero_eqz x, y)
and
(czero_eqz x, x) -> x

isel already does the (czero_eqz x, (setne y, 0)) -> (czero_eqz x, y) and (czero_nez x, (seteq y, 0)) -> (czero_eqz x, y) combines, but doing them early could expose other opportunities.


Full diff: https://github.com/llvm/llvm-project/pull/90428.diff

1 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+28-20)
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index 3ab9e7d69105ca..069465a53a03e8 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -16165,28 +16165,36 @@ SDValue RISCVTargetLowering::PerformDAGCombine(SDNode *N,
     return performSELECTCombine(N, DAG, Subtarget);
   case RISCVISD::CZERO_EQZ:
   case RISCVISD::CZERO_NEZ: {
-    SDValue LHS = N->getOperand(0);
-    SDValue RHS = N->getOperand(1);
-    // czero_eq X, (xor Y, 1) -> czero_ne X, Y if Y is 0 or 1.
-    // czero_ne X, (xor Y, 1) -> czero_eq X, Y if Y is 0 or 1.
-    if (RHS.getOpcode() == ISD::XOR && isOneConstant(RHS.getOperand(1))) {
-      SDValue Cond = RHS.getOperand(0);
+    SDValue Val = N->getOperand(0);
+    SDValue Cond = N->getOperand(1);
+
+    unsigned Opc = N->getOpcode();
+
+    // czero_eqz x, x -> x
+    if (Opc && Val == Cond)
+      return Val;
+
+    unsigned InvOpc =
+        Opc == RISCVISD::CZERO_EQZ ? RISCVISD::CZERO_NEZ : RISCVISD::CZERO_EQZ;
+
+    // czero_eqz X, (xor Y, 1) -> czero_nez X, Y if Y is 0 or 1.
+    // czero_nez X, (xor Y, 1) -> czero_eqz X, Y if Y is 0 or 1.
+    if (Cond.getOpcode() == ISD::XOR && isOneConstant(Cond.getOperand(1))) {
+      Cond = Cond.getOperand(0);
       APInt Mask = APInt::getBitsSetFrom(Cond.getValueSizeInBits(), 1);
-      if (DAG.MaskedValueIsZero(Cond, Mask)) {
-        unsigned NewOpc = N->getOpcode() == RISCVISD::CZERO_EQZ
-                              ? RISCVISD::CZERO_NEZ
-                              : RISCVISD::CZERO_EQZ;
-        return DAG.getNode(NewOpc, SDLoc(N), N->getValueType(0), LHS, Cond);
-      }
+      if (DAG.MaskedValueIsZero(Cond, Mask))
+        return DAG.getNode(InvOpc, SDLoc(N), N->getValueType(0), Val, Cond);
+    }
+    // czero_eqz x, (setcc y, 0, ne) -> czero_eqz x, y
+    // czero_nez x, (setcc y, 0, ne) -> czero_nez x, y
+    // czero_eqz x, (setcc y, 0, eq) -> czero_nez x, y
+    // czero_nez x, (setcc y, 0, eq) -> czero_eqz x, y
+    if (Cond.getOpcode() == ISD::SETCC && isNullConstant(Cond.getOperand(1))) {
+      ISD::CondCode CCVal = cast<CondCodeSDNode>(Cond.getOperand(2))->get();
+      if (ISD::isIntEqualitySetCC(CCVal))
+        return DAG.getNode(CCVal == ISD::SETNE ? Opc : InvOpc, SDLoc(N),
+                           N->getValueType(0), Val, Cond.getOperand(0));
     }
-    // czero_eqz x, (setcc x, 0, ne) -> x
-    // czero_nez x, (setcc x, 0, eq) -> x
-    if (RHS.getOpcode() == ISD::SETCC && isNullConstant(RHS.getOperand(1)) &&
-        cast<CondCodeSDNode>(RHS.getOperand(2))->get() ==
-            (N->getOpcode() == RISCVISD::CZERO_EQZ ? ISD::CondCode::SETNE
-                                                   : ISD::CondCode::SETEQ) &&
-        LHS == RHS.getOperand(0))
-      return LHS;
     return SDValue();
   }
   case RISCVISD::SELECT_CC: {

@topperc
Copy link
Collaborator Author

topperc commented Apr 29, 2024

CC @zengdage

llvm/lib/Target/RISCV/RISCVISelLowering.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/RISCV/RISCVISelLowering.cpp Show resolved Hide resolved
// czero_nez x, (setcc y, 0, ne) -> czero_nez x, y
// czero_eqz x, (setcc y, 0, eq) -> czero_nez x, y
// czero_nez x, (setcc y, 0, eq) -> czero_eqz x, y
if (Cond.getOpcode() == ISD::SETCC && isNullConstant(Cond.getOperand(1))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the following logic can be removed now.

def : Pat<(XLenVT (riscv_czero_eqz GPR:$rs1, (riscv_setne (XLenVT GPR:$rc)))),
(CZERO_EQZ GPR:$rs1, GPR:$rc)>;
def : Pat<(XLenVT (riscv_czero_eqz GPR:$rs1, (riscv_seteq (XLenVT GPR:$rc)))),
(CZERO_NEZ GPR:$rs1, GPR:$rc)>;
def : Pat<(XLenVT (riscv_czero_nez GPR:$rs1, (riscv_setne (XLenVT GPR:$rc)))),
(CZERO_NEZ GPR:$rs1, GPR:$rc)>;
def : Pat<(XLenVT (riscv_czero_nez GPR:$rs1, (riscv_seteq (XLenVT GPR:$rc)))),
(CZERO_EQZ GPR:$rs1, GPR:$rc)>;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those can't remove removed. They handle cases other than RHS being 0.

dtcxzyw added a commit to dtcxzyw/llvm-codegen-benchmark that referenced this pull request Apr 29, 2024
@dtcxzyw
Copy link
Member

dtcxzyw commented Apr 29, 2024

Hmm, this patch seems like a NFC(I) change :) If not, please add some tests.

Copy link
Member

@dtcxzyw dtcxzyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@topperc
Copy link
Collaborator Author

topperc commented Apr 29, 2024

Hmm, this patch seems like a NFC(I) change :) If not, please add some tests.

I don't think it's is quite NFC. Removing the setcc in DAGCombine can change use counts and enable other combines that may check that the setcc has only 1 use. I don't have a specific example in mind.

@topperc topperc merged commit f9d4d54 into llvm:main Apr 29, 2024
4 checks passed
@topperc topperc deleted the pr/czero branch April 29, 2024 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants