[GlobalIsel] Combine select of binops #76763

tschuett · 2024-01-02T23:12:09Z

No description provided.

llvmbot · 2024-01-02T23:12:38Z

@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-llvm-globalisel

Author: Thorsten Schütt (tschuett)

Changes

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

4 Files Affected:

(modified) llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h (+3)
(modified) llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h (+103)
(modified) llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp (+63-28)
(modified) llvm/test/CodeGen/AArch64/GlobalISel/combine-select.mir (+74)

diff --git a/llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h b/llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
index dcc1a4580b14a2..f3b68623596c46 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
@@ -910,6 +910,9 @@ class CombinerHelper {
 
   bool tryFoldSelectOfConstants(GSelect *Select, BuildFnTy &MatchInfo);
 
+  /// Try to fold select(cc, binop(), binop()) -> binop(select(), X)
+  bool tryFoldSelectOfBinOps(GSelect *Select, BuildFnTy &MatchInfo);
+
   bool isOneOrOneSplat(Register Src, bool AllowUndefs);
   bool isZeroOrZeroSplat(Register Src, bool AllowUndefs);
   bool isConstantSplatVector(Register Src, int64_t SplatValue,
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h b/llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
index 6ab1d4550c51ca..21d98d30356c93 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
@@ -558,6 +558,109 @@ class GVecReduce : public GenericMachineInstr {
   }
 };
 
+// Represents a binary operation, i.e, x = y op z.
+class GBinOp : public GenericMachineInstr {
+public:
+  Register getLHSReg() const { return getReg(1); }
+  Register getRHSReg() const { return getReg(2); }
+
+  static bool classof(const MachineInstr *MI) {
+    switch (MI->getOpcode()) {
+    // Integer.
+    case TargetOpcode::G_ADD:
+    case TargetOpcode::G_SUB:
+    case TargetOpcode::G_MUL:
+    case TargetOpcode::G_SDIV:
+    case TargetOpcode::G_UDIV:
+    case TargetOpcode::G_SREM:
+    case TargetOpcode::G_UREM:
+    case TargetOpcode::G_SMIN:
+    case TargetOpcode::G_SMAX:
+    case TargetOpcode::G_UMIN:
+    case TargetOpcode::G_UMAX:
+    // Floating point.
+    case TargetOpcode::G_FMINNUM:
+    case TargetOpcode::G_FMAXNUM:
+    case TargetOpcode::G_FMINNUM_IEEE:
+    case TargetOpcode::G_FMAXNUM_IEEE:
+    case TargetOpcode::G_FMINIMUM:
+    case TargetOpcode::G_FMAXIMUM:
+    case TargetOpcode::G_FADD:
+    case TargetOpcode::G_FSUB:
+    case TargetOpcode::G_FMUL:
+    case TargetOpcode::G_FDIV:
+    case TargetOpcode::G_FPOW:
+    // Logical.
+    case TargetOpcode::G_AND:
+    case TargetOpcode::G_OR:
+    case TargetOpcode::G_XOR:
+      return true;
+    default:
+      return false;
+    }
+  };
+};
+
+// Represents an integer binary operation.
+class GIntBinOp : public GBinOp {
+public:
+  static bool classof(const MachineInstr *MI) {
+    switch (MI->getOpcode()) {
+    case TargetOpcode::G_ADD:
+    case TargetOpcode::G_SUB:
+    case TargetOpcode::G_MUL:
+    case TargetOpcode::G_SDIV:
+    case TargetOpcode::G_UDIV:
+    case TargetOpcode::G_SREM:
+    case TargetOpcode::G_UREM:
+    case TargetOpcode::G_SMIN:
+    case TargetOpcode::G_SMAX:
+    case TargetOpcode::G_UMIN:
+    case TargetOpcode::G_UMAX:
+      return true;
+    default:
+      return false;
+    }
+  };
+};
+
+// Represents a floating point binary operation.
+class GFBinOp : public GBinOp {
+public:
+  static bool classof(const MachineInstr *MI) {
+    switch (MI->getOpcode()) {
+    case TargetOpcode::G_FMINNUM:
+    case TargetOpcode::G_FMAXNUM:
+    case TargetOpcode::G_FMINNUM_IEEE:
+    case TargetOpcode::G_FMAXNUM_IEEE:
+    case TargetOpcode::G_FMINIMUM:
+    case TargetOpcode::G_FMAXIMUM:
+    case TargetOpcode::G_FADD:
+    case TargetOpcode::G_FSUB:
+    case TargetOpcode::G_FMUL:
+    case TargetOpcode::G_FDIV:
+    case TargetOpcode::G_FPOW:
+      return true;
+    default:
+      return false;
+    }
+  };
+};
+
+// Represents a logical binary operation.
+class GLogicalBinOp : public GBinOp {
+public:
+  static bool classof(const MachineInstr *MI) {
+    switch (MI->getOpcode()) {
+    case TargetOpcode::G_AND:
+    case TargetOpcode::G_OR:
+    case TargetOpcode::G_XOR:
+      return true;
+    default:
+      return false;
+    }
+  };
+};
 
 } // namespace llvm
 
diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index 8b15bdb0aca30b..102b49c48460b1 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -6390,8 +6390,7 @@ bool CombinerHelper::tryFoldSelectOfConstants(GSelect *Select,
   if (TrueValue.isZero() && FalseValue.isOne()) {
     MatchInfo = [=](MachineIRBuilder &B) {
       B.setInstrAndDebugLoc(*Select);
-      Register Inner = MRI.createGenericVirtualRegister(CondTy);
-      B.buildNot(Inner, Cond);
+      auto Inner = B.buildNot(CondTy, Cond);
       B.buildZExtOrTrunc(Dest, Inner);
     };
     return true;
@@ -6401,8 +6400,7 @@ bool CombinerHelper::tryFoldSelectOfConstants(GSelect *Select,
   if (TrueValue.isZero() && FalseValue.isAllOnes()) {
     MatchInfo = [=](MachineIRBuilder &B) {
       B.setInstrAndDebugLoc(*Select);
-      Register Inner = MRI.createGenericVirtualRegister(CondTy);
-      B.buildNot(Inner, Cond);
+      auto Inner = B.buildNot(CondTy, Cond);
       B.buildSExtOrTrunc(Dest, Inner);
     };
     return true;
@@ -6412,8 +6410,7 @@ bool CombinerHelper::tryFoldSelectOfConstants(GSelect *Select,
   if (TrueValue - 1 == FalseValue) {
     MatchInfo = [=](MachineIRBuilder &B) {
       B.setInstrAndDebugLoc(*Select);
-      Register Inner = MRI.createGenericVirtualRegister(TrueTy);
-      B.buildZExtOrTrunc(Inner, Cond);
+      auto Inner = B.buildZExtOrTrunc(TrueTy, Cond);
       B.buildAdd(Dest, Inner, False);
     };
     return true;
@@ -6423,8 +6420,7 @@ bool CombinerHelper::tryFoldSelectOfConstants(GSelect *Select,
   if (TrueValue + 1 == FalseValue) {
     MatchInfo = [=](MachineIRBuilder &B) {
       B.setInstrAndDebugLoc(*Select);
-      Register Inner = MRI.createGenericVirtualRegister(TrueTy);
-      B.buildSExtOrTrunc(Inner, Cond);
+      auto Inner = B.buildSExtOrTrunc(TrueTy, Cond);
       B.buildAdd(Dest, Inner, False);
     };
     return true;
@@ -6434,8 +6430,7 @@ bool CombinerHelper::tryFoldSelectOfConstants(GSelect *Select,
   if (TrueValue.isPowerOf2() && FalseValue.isZero()) {
     MatchInfo = [=](MachineIRBuilder &B) {
       B.setInstrAndDebugLoc(*Select);
-      Register Inner = MRI.createGenericVirtualRegister(TrueTy);
-      B.buildZExtOrTrunc(Inner, Cond);
+      auto Inner = B.buildZExtOrTrunc(TrueTy, Cond);
       // The shift amount must be scalar.
       LLT ShiftTy = TrueTy.isVector() ? TrueTy.getElementType() : TrueTy;
       auto ShAmtC = B.buildConstant(ShiftTy, TrueValue.exactLogBase2());
@@ -6447,8 +6442,7 @@ bool CombinerHelper::tryFoldSelectOfConstants(GSelect *Select,
   if (TrueValue.isAllOnes()) {
     MatchInfo = [=](MachineIRBuilder &B) {
       B.setInstrAndDebugLoc(*Select);
-      Register Inner = MRI.createGenericVirtualRegister(TrueTy);
-      B.buildSExtOrTrunc(Inner, Cond);
+      auto Inner = B.buildSExtOrTrunc(TrueTy, Cond);
       B.buildOr(Dest, Inner, False, Flags);
     };
     return true;
@@ -6458,10 +6452,8 @@ bool CombinerHelper::tryFoldSelectOfConstants(GSelect *Select,
   if (FalseValue.isAllOnes()) {
     MatchInfo = [=](MachineIRBuilder &B) {
       B.setInstrAndDebugLoc(*Select);
-      Register Not = MRI.createGenericVirtualRegister(CondTy);
-      B.buildNot(Not, Cond);
-      Register Inner = MRI.createGenericVirtualRegister(TrueTy);
-      B.buildSExtOrTrunc(Inner, Not);
+      auto Not = B.buildNot(CondTy, Cond);
+      auto Inner = B.buildSExtOrTrunc(TrueTy, Not);
       B.buildOr(Dest, Inner, True, Flags);
     };
     return true;
@@ -6496,8 +6488,7 @@ bool CombinerHelper::tryFoldBoolSelectToLogic(GSelect *Select,
   if ((Cond == True) || isOneOrOneSplat(True, /* AllowUndefs */ true)) {
     MatchInfo = [=](MachineIRBuilder &B) {
       B.setInstrAndDebugLoc(*Select);
-      Register Ext = MRI.createGenericVirtualRegister(TrueTy);
-      B.buildZExtOrTrunc(Ext, Cond);
+      auto Ext = B.buildZExtOrTrunc(TrueTy, Cond);
       B.buildOr(DstReg, Ext, False, Flags);
     };
     return true;
@@ -6508,8 +6499,7 @@ bool CombinerHelper::tryFoldBoolSelectToLogic(GSelect *Select,
   if ((Cond == False) || isZeroOrZeroSplat(False, /* AllowUndefs */ true)) {
     MatchInfo = [=](MachineIRBuilder &B) {
       B.setInstrAndDebugLoc(*Select);
-      Register Ext = MRI.createGenericVirtualRegister(TrueTy);
-      B.buildZExtOrTrunc(Ext, Cond);
+      auto Ext = B.buildZExtOrTrunc(TrueTy, Cond);
       B.buildAnd(DstReg, Ext, True);
     };
     return true;
@@ -6520,11 +6510,9 @@ bool CombinerHelper::tryFoldBoolSelectToLogic(GSelect *Select,
     MatchInfo = [=](MachineIRBuilder &B) {
       B.setInstrAndDebugLoc(*Select);
       // First the not.
-      Register Inner = MRI.createGenericVirtualRegister(CondTy);
-      B.buildNot(Inner, Cond);
+      auto Inner = B.buildNot(CondTy, Cond);
       // Then an ext to match the destination register.
-      Register Ext = MRI.createGenericVirtualRegister(TrueTy);
-      B.buildZExtOrTrunc(Ext, Inner);
+      auto Ext = B.buildZExtOrTrunc(TrueTy, Inner);
       B.buildOr(DstReg, Ext, True, Flags);
     };
     return true;
@@ -6535,11 +6523,9 @@ bool CombinerHelper::tryFoldBoolSelectToLogic(GSelect *Select,
     MatchInfo = [=](MachineIRBuilder &B) {
       B.setInstrAndDebugLoc(*Select);
       // First the not.
-      Register Inner = MRI.createGenericVirtualRegister(CondTy);
-      B.buildNot(Inner, Cond);
+      auto Inner = B.buildNot(CondTy, Cond);
       // Then an ext to match the destination register.
-      Register Ext = MRI.createGenericVirtualRegister(TrueTy);
-      B.buildZExtOrTrunc(Ext, Inner);
+      auto Ext = B.buildZExtOrTrunc(TrueTy, Inner);
       B.buildAnd(DstReg, Ext, False);
     };
     return true;
@@ -6548,6 +6534,52 @@ bool CombinerHelper::tryFoldBoolSelectToLogic(GSelect *Select,
   return false;
 }
 
+bool CombinerHelper::tryFoldSelectOfBinOps(GSelect *Select,
+                                           BuildFnTy &MatchInfo) {
+  Register DstReg = Select->getReg(0);
+  Register Cond = Select->getCondReg();
+  Register False = Select->getFalseReg();
+  Register True = Select->getTrueReg();
+  LLT DstTy = MRI.getType(DstReg);
+
+  GBinOp *LHS = getOpcodeDef<GBinOp>(True, MRI);
+  GBinOp *RHS = getOpcodeDef<GBinOp>(False, MRI);
+
+  // We need two binops of the same kind on the true/false registers.
+  if (!LHS || !RHS || LHS->getOpcode() != RHS->getOpcode())
+    return false;
+
+  // Note that there are no constraints on CondTy.
+  unsigned Flags = LHS->getFlags() & RHS->getFlags();
+  unsigned Opcode = LHS->getOpcode();
+
+  // Fold select(cond, binop(x, y), binop(z, y))
+  //  --> binop(select(cond, x, z), y)
+  if (LHS->getRHSReg() == RHS->getRHSReg()) {
+    MatchInfo = [=](MachineIRBuilder &B) {
+      B.setInstrAndDebugLoc(*Select);
+      auto Sel = B.buildSelect(DstTy, Cond, LHS->getLHSReg(), RHS->getLHSReg());
+      B.buildInstr(Opcode, {DstReg}, {Sel, LHS->getRHSReg()}, Flags);
+    };
+    return true;
+  }
+
+  // Fold select(cond, binop(x, y), binop(x, z))
+  //  --> binop(x, select(cond, y, z))
+  if (LHS->getLHSReg() == RHS->getLHSReg()) {
+    MatchInfo = [=](MachineIRBuilder &B) {
+      B.setInstrAndDebugLoc(*Select);
+      auto Sel = B.buildSelect(DstTy, Cond, LHS->getRHSReg(), RHS->getRHSReg());
+      B.buildInstr(Opcode, {DstReg}, {LHS->getLHSReg(), Sel}, Flags);
+    };
+    return true;
+  }
+
+  // FIXME: use isCommutable().
+
+  return false;
+}
+
 bool CombinerHelper::matchSelect(MachineInstr &MI, BuildFnTy &MatchInfo) {
   GSelect *Select = cast<GSelect>(&MI);
 
@@ -6557,5 +6589,8 @@ bool CombinerHelper::matchSelect(MachineInstr &MI, BuildFnTy &MatchInfo) {
   if (tryFoldBoolSelectToLogic(Select, MatchInfo))
     return true;
 
+  if (tryFoldSelectOfBinOps(Select, MatchInfo))
+    return true;
+
   return false;
 }
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/combine-select.mir b/llvm/test/CodeGen/AArch64/GlobalISel/combine-select.mir
index be2de620fa456c..7644443f53dc68 100644
--- a/llvm/test/CodeGen/AArch64/GlobalISel/combine-select.mir
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/combine-select.mir
@@ -544,3 +544,77 @@ body:             |
     %ext:_(s32) = G_ANYEXT %sel
     $w0 = COPY %ext(s32)
 ...
+---
+# select cond, and(x, y), and(z, y) --> and (select, x, z), y
+name:            select_cond_and_x_y_and_z_y_and_select_x_z_y
+body:             |
+  bb.1:
+    liveins: $x0, $x1, $x2
+    ; CHECK-LABEL: name: select_cond_and_x_y_and_z_y_and_select_x_z_y
+    ; CHECK: liveins: $x0, $x1, $x2
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s64) = COPY $x0
+    ; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s64) = COPY $x1
+    ; CHECK-NEXT: [[COPY2:%[0-9]+]]:_(s64) = COPY $x2
+    ; CHECK-NEXT: [[COPY3:%[0-9]+]]:_(s64) = COPY $x3
+    ; CHECK-NEXT: %c:_(s1) = G_TRUNC [[COPY]](s64)
+    ; CHECK-NEXT: %a:_(s8) = G_TRUNC [[COPY1]](s64)
+    ; CHECK-NEXT: %b:_(s8) = G_TRUNC [[COPY2]](s64)
+    ; CHECK-NEXT: %d:_(s8) = G_TRUNC [[COPY3]](s64)
+    ; CHECK-NEXT: [[SELECT:%[0-9]+]]:_(s8) = G_SELECT %c(s1), %a, %d
+    ; CHECK-NEXT: %sel:_(s8) = G_AND [[SELECT]], %b
+    ; CHECK-NEXT: %ext:_(s32) = G_ANYEXT %sel(s8)
+    ; CHECK-NEXT: $w0 = COPY %ext(s32)
+    %0:_(s64) = COPY $x0
+    %1:_(s64) = COPY $x1
+    %2:_(s64) = COPY $x2
+    %3:_(s64) = COPY $x3
+    %4:_(s64) = COPY $x4
+    %c:_(s1) = G_TRUNC %0
+    %a:_(s8) = G_TRUNC %1
+    %b:_(s8) = G_TRUNC %2
+    %d:_(s8) = G_TRUNC %3
+    %e:_(s8) = G_TRUNC %4
+    %and1:_(s8) = G_AND %a, %b
+    %and2:_(s8) = G_AND %d, %b
+    %sel:_(s8) = G_SELECT %c, %and1, %and2
+    %ext:_(s32) = G_ANYEXT %sel
+    $w0 = COPY %ext(s32)
+...
+---
+# select cond, xor(x, y), xor(x, z) --> xor x, select, x, z)
+name:            select_cond_xor_x_y_xor_x_z_xor_x__select_x_y
+body:             |
+  bb.1:
+    liveins: $x0, $x1, $x2
+    ; CHECK-LABEL: name: select_cond_xor_x_y_xor_x_z_xor_x__select_x_y
+    ; CHECK: liveins: $x0, $x1, $x2
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s64) = COPY $x0
+    ; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s64) = COPY $x1
+    ; CHECK-NEXT: [[COPY2:%[0-9]+]]:_(s64) = COPY $x3
+    ; CHECK-NEXT: [[COPY3:%[0-9]+]]:_(s64) = COPY $x4
+    ; CHECK-NEXT: %c:_(s1) = G_TRUNC [[COPY]](s64)
+    ; CHECK-NEXT: %a:_(s8) = G_TRUNC [[COPY1]](s64)
+    ; CHECK-NEXT: %d:_(s8) = G_TRUNC [[COPY2]](s64)
+    ; CHECK-NEXT: %e:_(s8) = G_TRUNC [[COPY3]](s64)
+    ; CHECK-NEXT: [[SELECT:%[0-9]+]]:_(s8) = G_SELECT %c(s1), %e, %d
+    ; CHECK-NEXT: %sel:_(s8) = G_XOR %a, [[SELECT]]
+    ; CHECK-NEXT: %ext:_(s32) = G_ANYEXT %sel(s8)
+    ; CHECK-NEXT: $w0 = COPY %ext(s32)
+    %0:_(s64) = COPY $x0
+    %1:_(s64) = COPY $x1
+    %2:_(s64) = COPY $x2
+    %3:_(s64) = COPY $x3
+    %4:_(s64) = COPY $x4
+    %c:_(s1) = G_TRUNC %0
+    %a:_(s8) = G_TRUNC %1
+    %b:_(s8) = G_TRUNC %2
+    %d:_(s8) = G_TRUNC %3
+    %e:_(s8) = G_TRUNC %4
+    %xor1:_(s8) = G_XOR %a, %e
+    %xor2:_(s8) = G_XOR %a, %d
+    %sel:_(s8) = G_SELECT %c, %xor1, %xor2
+    %ext:_(s32) = G_ANYEXT %sel
+    $w0 = COPY %ext(s32)
+...

tschuett · 2024-01-02T23:13:14Z

This one has a limited effect. I fixed the coding style violations from the last PR.

arsenm · 2024-01-03T05:20:48Z

llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h

+  Register getRHSReg() const { return getReg(2); }
+
+  static bool classof(const MachineInstr *MI) {
+    switch (MI->getOpcode()) {


Could you implement this in terms of GIntBinOp::classof and GFBinOp::classof to avoid listing the opcodes in multiple places?

I was going to suggest that too but it won't compile due to not yet seeing the definition of GIntBinOp when parsing this class.

arsenm · 2024-01-03T05:21:48Z

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

+    return false;
+
+  // Note that there are no constraints on CondTy.
+  unsigned Flags = LHS->getFlags() & RHS->getFlags();


This doesn't consider the flags on the select itself

The DAG combiner intersects the flags of the binops and ignores the select flags.

That's also suboptimal for the DAG combiner. I think it should be (LHS & RHS) | Select. Does InstCombine preserve these?

It's also quite likely that code was never properly updated for select gaining flags. It was a relatively recent IR change

NewBinOp->setFlags(N1->getFlags()); NewBinOp->intersectFlagsWith(N2->getFlags());

It ignores the select flags. I updated my code locally.

arsenm · 2024-01-03T05:22:38Z

llvm/test/CodeGen/AArch64/GlobalISel/combine-select.mir

+    %e:_(s8) = G_TRUNC %4
+    %xor1:_(s8) = G_XOR %a, %e
+    %xor2:_(s8) = G_XOR %a, %d
+    %sel:_(s8) = G_SELECT %c, %xor1, %xor2


Could use negative test with mismatched opcodes. Also missing tests for the flag handling. Precommitting tests would also be good

aemerson · 2024-01-03T05:36:43Z

llvm/test/CodeGen/AArch64/GlobalISel/combine-select.mir

@@ -544,3 +544,77 @@ body:             |
    %ext:_(s32) = G_ANYEXT %sel
    $w0 = COPY %ext(s32)
 ...
+---
+# select cond, and(x, y), and(z, y) --> and (select, x, z), y


nit: comment missing cond on RHS: --> and (select cond, x, z), y

aemerson

Need to address Matt's comment.

aemerson · 2024-01-03T05:41:14Z

llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h

+  Register getRHSReg() const { return getReg(2); }
+
+  static bool classof(const MachineInstr *MI) {
+    switch (MI->getOpcode()) {


I was going to suggest that too but it won't compile due to not yet seeing the definition of GIntBinOp when parsing this class.

…ject into gisel-select-of-binops

tschuett · 2024-01-03T14:50:03Z

Please review changes (C++ and mir) regarding flag handling.

tschuett · 2024-01-03T15:43:55Z

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

+    return false;
+
+  // Note that there are no constraints on CondTy.
+  unsigned Flags = (LHS->getFlags() & RHS->getFlags()) | Select->getFlags();


This line differs from the Dag combiner.

I would have split this by matching the DAG behavior in the initial commit but doesn't really matter. I think this is OK but would be nice to have alive verify

tschuett · 2024-01-03T15:44:29Z

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

+    MatchInfo = [=](MachineIRBuilder &B) {
+      B.setInstrAndDebugLoc(*Select);
+      auto Sel = B.buildSelect(DstTy, Cond, LHS->getLHSReg(), RHS->getLHSReg(),
+                               Select->getFlags());


And Flags are added to selects.

tschuett · 2024-01-03T15:45:38Z

llvm/test/CodeGen/AArch64/GlobalISel/combine-select.mir

+    ; CHECK-NEXT: %d:_(s8) = G_TRUNC [[COPY2]](s64)
+    ; CHECK-NEXT: %e:_(s8) = G_TRUNC [[COPY3]](s64)
+    ; CHECK-NEXT: [[SELECT:%[0-9]+]]:_(s8) = ninf exact G_SELECT %c(s1), %e, %d
+    ; CHECK-NEXT: %sel:_(s8) = ninf arcp exact G_XOR %a, [[SELECT]]


G_XOR has arcp because both ors had it. And it got exact from the select.

tschuett · 2024-01-03T15:46:06Z

llvm/test/CodeGen/AArch64/GlobalISel/combine-select.mir

+    %d:_(s8) = G_TRUNC %3
+    %e:_(s8) = G_TRUNC %4
+    %and1:_(s8) = G_AND %a, %b
+    %or2:_(s8) = G_OR %e, %d


This failed because and and or.

ronlieb · 2024-01-06T15:07:17Z

this lit test is now taking a very long time after this patch landed
./bin/llvm-lit ../llvm/test/CodeGen/AMDGPU/llvm.log2.ll

relevant cmake settings:
-DCMAKE_BUILD_TYPE=Release
-DLLVM_ENABLE_PROJECTS="clang;lld;llvm;flang"
-DLLVM_TARGETS_TO_BUILD="X86;AMDGPU"
-DLLVM_ENABLE_ASSERTIONS=ON
-DLLVM_ENABLE_RUNTIMES="openmp;compiler-rt"
-DCLANG_DEFAULT_LINKER=lld \

ronlieb · 2024-01-06T15:08:38Z

@jhuber6 @jplehr this may also be failing in staging libc bot https://lab.llvm.org/staging/#/builders/134

tschuett · 2024-01-06T15:12:20Z

I had also issues with this test. There is also a GH issue: #76821.

tschuett · 2024-01-06T15:14:56Z

The file was not changed by this PR and there is no select in the file.

bzEq · 2024-01-06T15:18:00Z

It's also exhausting memory when llvm-lit llvm-project/llvm/test/CodeGen/AMDGPU/llvm.exp2.ll.
See https://lab.llvm.org/buildbot/#/builders/249/builds/13786.

This reverts commit 1687555.

tschuett · 2024-01-06T16:05:28Z

I reverted the commit.

This reverts commit 1687555.

[GlobalIsel] Combine select of binops

e713bb6

llvmbot added backend:AArch64 llvm:globalisel labels Jan 2, 2024

tschuett requested review from aemerson, arsenm and topperc January 2, 2024 23:12

arsenm reviewed Jan 3, 2024

View reviewed changes

aemerson approved these changes Jan 3, 2024

View reviewed changes

aemerson requested changes Jan 3, 2024

View reviewed changes

tschuett mentioned this pull request Jan 3, 2024

llc crashed in buildkite #76821

Closed

tschuett added 4 commits January 3, 2024 15:41

[GlobalIsel] Combine select of binops

4eab254

address comments

85ef024

Merge branch 'llvm:main' into gisel-select-of-binops

8f4734a

Merge branch 'gisel-select-of-binops' of github.com:tschuett/llvm-pro…

73dfaf4

…ject into gisel-select-of-binops

fix select flags

b6ee5a5

tschuett commented Jan 3, 2024

View reviewed changes

arsenm approved these changes Jan 4, 2024

View reviewed changes

aemerson approved these changes Jan 6, 2024

View reviewed changes

tschuett merged commit 1687555 into llvm:main Jan 6, 2024
3 of 4 checks passed

tschuett deleted the gisel-select-of-binops branch January 6, 2024 10:28

tschuett added a commit that referenced this pull request Jan 6, 2024

Revert "[GlobalIsel] Combine select of binops (#76763)"

a085402

This reverts commit 1687555.

justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024

[GlobalIsel] Combine select of binops (llvm#76763)

4fe1f90

justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024

Revert "[GlobalIsel] Combine select of binops (llvm#76763)"

27b0233

This reverts commit 1687555.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GlobalIsel] Combine select of binops #76763

[GlobalIsel] Combine select of binops #76763

tschuett commented Jan 2, 2024

llvmbot commented Jan 2, 2024 •

edited

tschuett commented Jan 2, 2024

arsenm Jan 3, 2024

aemerson Jan 3, 2024

arsenm Jan 3, 2024

tschuett Jan 3, 2024

arsenm Jan 3, 2024

arsenm Jan 3, 2024

tschuett Jan 3, 2024

arsenm Jan 3, 2024

aemerson Jan 3, 2024

aemerson left a comment

aemerson Jan 3, 2024

tschuett commented Jan 3, 2024

tschuett Jan 3, 2024

arsenm Jan 4, 2024

tschuett Jan 3, 2024

tschuett Jan 3, 2024

tschuett Jan 3, 2024

ronlieb commented Jan 6, 2024

ronlieb commented Jan 6, 2024

tschuett commented Jan 6, 2024

tschuett commented Jan 6, 2024

bzEq commented Jan 6, 2024

tschuett commented Jan 6, 2024

[GlobalIsel] Combine select of binops #76763

[GlobalIsel] Combine select of binops #76763

Conversation

tschuett commented Jan 2, 2024

llvmbot commented Jan 2, 2024 • edited

tschuett commented Jan 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aemerson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschuett commented Jan 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronlieb commented Jan 6, 2024

ronlieb commented Jan 6, 2024

tschuett commented Jan 6, 2024

tschuett commented Jan 6, 2024

bzEq commented Jan 6, 2024

tschuett commented Jan 6, 2024

llvmbot commented Jan 2, 2024 •

edited