[AArch64] Add new target feature to fuse conditional select

[evandro]

Summary

This feature enables the fusion of the comparison and the conditional select instructions together.

[fhahn]

llvm/lib/Target/AArch64/AArch64MacroFusion.cpp

158 ↗

(On Diff #130943)

Do I understand correctly and are all the changes from the top to here just to avoid returning false early? I think what the existing patterns do is: Either check the first or second opcode. If it's a match, check if the other opcode can be fused with it and return true/false. If the first check is fails, continue. After a quick look, I do not think any of the first checks overlap with the new checks you added in this patch (i.e. no other pattern checks if the SecondOpcode in [ CSELWr, CSELXr ]), so is it really necessary to change the logic above?

On a more general note, it seems like the list of patterns is growing quite large (especially together with D42393). I think going forward it would make it easier if we move the pattern checks in helper functions, where we can just have the switch's and early returns. So in shouldScheduleAdjacent we would just have something like

if (ST.hasFuseAES() && isAESPattern(Op1, Op2)) return true;
if (ST.hasArithmeticCbzFusion() && isCbzFusionPattern(Op1, Op2)) return true;
.....
if (ST.hasFuseCCSelect() && isCCSelectPattern) return true;

IMO this would be easier to read understand and maintain. That's just my personal opinion though, not sure how other people feel.

We could even get rid of all the ifs if we want and just have a single return with all the patterns matching functions we support.

[evandro]

llvm/lib/Target/AArch64/AArch64MacroFusion.cpp

158 ↗

(On Diff #130943)

The change is needed because otherwise the default exit is premature, as other tests may match conditions that failed in one test.

I agree that this is growing unwieldy. Perhaps the time to refactor this code in separate helper functions is nigh. However, I also dislike how hard it is to add new fused pairs. That's why I proposed D35228, which would simplify adding and maintaining fusion quite a bit IMO. Hopefully, I'll be able to round it out as @hfinkel suggested in the coming weeks.

[evandro]

Ping! 🔔

[evandro]

¡Ping! 🔔🔔

[evandro]

Ping!!! 🔔🔔🔔

[evandro]

Updated patch considering the refactoring in D43649.

[fhahn]

Thanks Evandro! LGTM.

llvm/lib/Target/AArch64/AArch64MacroFusion.cpp

230 ↗

(On Diff #135552)

No need for an if here I think, you could just return !AArch64InstrInfo::hasShiftedReg(*FirstMI) ?

234 ↗

(On Diff #135552)

Same as above

251 ↗

(On Diff #135552)

same as above

256 ↗

(On Diff #135552)

same as above

[evandro]

llvm/lib/Target/AArch64/AArch64MacroFusion.cpp

230 ↗

(On Diff #135552)

Of course!

llvm/trunk/lib/Target/AArch64/AArch64.td

 def FeatureFuseAddress : SubtargetFeature<
     "fuse-address", "HasFuseAddress", "true",
     "CPU fuses address generation and memory operations">;
 
 def FeatureFuseAES : SubtargetFeature<
     "fuse-aes", "HasFuseAES", "true",
     "CPU fuses AES crypto operations">;
 
+def FeatureFuseCCSelect : SubtargetFeature<
+    "fuse-csel", "HasFuseCCSelect", "true",
+    "CPU fuses conditional select operations">;
+
 def FeatureFuseLiterals : SubtargetFeature<
     "fuse-literals", "HasFuseLiterals", "true",
     "CPU fuses literal generation operations">;
 
 def FeatureDisableLatencySchedHeuristic : SubtargetFeature<
     "disable-latency-sched-heuristic", "DisableLatencySchedHeuristic", "true",
     "Disable latency scheduling heuristic">;
 

 def ProcExynosM3 : SubtargetFeature<"exynosm3", "ARMProcFamily", "ExynosM3",
                                     "Samsung Exynos-M3 processors",
                                     [FeatureCRC,
                                      FeatureCrypto,
                                      FeatureExynosCheapAsMoveHandling,
                                      FeatureFPARMv8,
                                      FeatureFuseAddress,
                                      FeatureFuseAES,
+                                     FeatureFuseCCSelect,
                                      FeatureFuseLiterals,
                                      FeatureLSLFast,
                                      FeatureNEON,
                                      FeaturePerfMon,
                                      FeaturePostRAScheduler,
                                      FeaturePredictableSelectIsExpensive,
                                      FeatureZCZeroing]>;
 

llvm/trunk/lib/Target/AArch64/AArch64MacroFusion.cpp

if (SecondMI.getOpcode() == AArch64::Bcc) {
     case AArch64::ADDSXrs:
     case AArch64::ANDSWrs:
     case AArch64::ANDSXrs:
     case AArch64::SUBSWrs:
     case AArch64::SUBSXrs:
     case AArch64::BICSWrs:
     case AArch64::BICSXrs:
       // Shift value can be 0 making these behave like the "rr" variant...
-      if (!AArch64InstrInfo::hasShiftedReg(*FirstMI))
-        return true;
+      return (!AArch64InstrInfo::hasShiftedReg(*FirstMI));
     }
   }
   return false;
 }
 
 // Fuse ALU operations followed by CBZ/CBNZ.
 static bool isArithmeticCbzPair(const MachineInstr *FirstMI,
                                 const MachineInstr &SecondMI) {
if (SecondOpcode == AArch64::CBNZW || SecondOpcode == AArch64::CBNZX ||
     case AArch64::ADDXrs:
     case AArch64::ANDWrs:
     case AArch64::ANDXrs:
     case AArch64::SUBWrs:
     case AArch64::SUBXrs:
     case AArch64::BICWrs:
     case AArch64::BICXrs:
       // Shift value can be 0 making these behave like the "rr" variant...
-      if (!AArch64InstrInfo::hasShiftedReg(*FirstMI))
-        return true;
+      return (!AArch64InstrInfo::hasShiftedReg(*FirstMI));
     }
   }
   return false;
 }
 
 // Fuse AES crypto encoding or decoding.
 static bool isAESPair(const MachineInstr *FirstMI,
                       const MachineInstr &SecondMI) {
   // Assume the 1st instr to be a wildcard if it is unspecified.
   unsigned FirstOpcode =
       FirstMI ? FirstMI->getOpcode()
               : static_cast<unsigned>(AArch64::INSTRUCTION_LIST_END);
   unsigned SecondOpcode = SecondMI.getOpcode();
 
   // AES encode.
   if ((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
        FirstOpcode == AArch64::AESErr) &&
       (SecondOpcode == AArch64::AESMCrr ||
        SecondOpcode == AArch64::AESMCrrTied))
     return true;
   // AES decode.
-  if ((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
+  else if ((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
             FirstOpcode == AArch64::AESDrr) &&
            (SecondOpcode == AArch64::AESIMCrr ||
             SecondOpcode == AArch64::AESIMCrrTied))
     return true;
 
   return false;
 }
 
 // Fuse literal generation.
 static bool isLiteralsPair(const MachineInstr *FirstMI,
                            const MachineInstr &SecondMI) {
   // Assume the 1st instr to be a wildcard if it is unspecified.
   unsigned FirstOpcode =
       FirstMI ? FirstMI->getOpcode()
               : static_cast<unsigned>(AArch64::INSTRUCTION_LIST_END);
   unsigned SecondOpcode = SecondMI.getOpcode();
 
   // PC relative address.
   if ((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
        FirstOpcode == AArch64::ADRP) &&
       SecondOpcode == AArch64::ADDXri)
     return true;
   // 32 bit immediate.
-  if ((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
+  else if ((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
             FirstOpcode == AArch64::MOVZWi) &&
-      SecondOpcode == AArch64::MOVKWi && SecondMI.getOperand(3).getImm() == 16)
+           (SecondOpcode == AArch64::MOVKWi &&
+            SecondMI.getOperand(3).getImm() == 16))
     return true;
   // Lower half of 64 bit immediate.
-  if((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
+  else if((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
            FirstOpcode == AArch64::MOVZXi) &&
-     SecondOpcode == AArch64::MOVKXi && SecondMI.getOperand(3).getImm() == 16)
+          (SecondOpcode == AArch64::MOVKXi &&
+           SecondMI.getOperand(3).getImm() == 16))
     return true;
   // Upper half of 64 bit immediate.
-  if ((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
+  else if ((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
             (FirstOpcode == AArch64::MOVKXi &&
              FirstMI->getOperand(3).getImm() == 32)) &&
-      SecondOpcode == AArch64::MOVKXi && SecondMI.getOperand(3).getImm() == 48)
+           (SecondOpcode == AArch64::MOVKXi &&
+            SecondMI.getOperand(3).getImm() == 48))
     return true;
 
   return false;
 }
 
 // Fuse address generation and loads or stores.
 static bool isAddressLdStPair(const MachineInstr *FirstMI,
                               const MachineInstr &SecondMI) {
static bool isAddressLdStPair(const MachineInstr *FirstMI,
   case AArch64::LDRSHXui:
   case AArch64::LDRSWui:
     // Assume the 1st instr to be a wildcard if it is unspecified.
     if (!FirstMI)
       return true;
 
     switch (FirstMI->getOpcode()) {
     case AArch64::ADR:
-      if (SecondMI.getOperand(2).getImm() == 0)
-        return true;
-      return false;
-    case AArch64::ADRP:
+      return (SecondMI.getOperand(2).getImm() == 0);
+    case AArch64::ADRP:
+    }
+  }
+}
+
+// Fuse compare and conditional select.
+static bool isCCSelectPair(const MachineInstr *FirstMI,
+                           const MachineInstr &SecondMI) {
+  unsigned SecondOpcode = SecondMI.getOpcode();
+
+  // 32 bits
+  if (SecondOpcode == AArch64::CSELWr) {
+    // Assume the 1st instr to be a wildcard if it is unspecified.
+    if (!FirstMI)
+      return true;
+
+    if (FirstMI->definesRegister(AArch64::WZR))
+      switch (FirstMI->getOpcode()) {
+      case AArch64::SUBSWrs:
+        return (!AArch64InstrInfo::hasShiftedReg(*FirstMI));
+      case AArch64::SUBSWrx:
+        return (!AArch64InstrInfo::hasExtendedReg(*FirstMI));
+      case AArch64::SUBSWrr:
+      case AArch64::SUBSWri:
+        return true;
+      }
+  }
+  // 64 bits
+  else if (SecondOpcode == AArch64::CSELXr) {
+    // Assume the 1st instr to be a wildcard if it is unspecified.
+    if (!FirstMI)
+      return true;
+
+    if (FirstMI->definesRegister(AArch64::XZR))
+      switch (FirstMI->getOpcode()) {
+      case AArch64::SUBSXrs:
+        return (!AArch64InstrInfo::hasShiftedReg(*FirstMI));
+      case AArch64::SUBSXrx:
+      case AArch64::SUBSXrx64:
+        return (!AArch64InstrInfo::hasExtendedReg(*FirstMI));
+      case AArch64::SUBSXrr:
+      case AArch64::SUBSXri:
         return true;
       }
   }
   return false;
 }
 
 /// \brief Check if the instr pair, FirstMI and SecondMI, should be fused
 /// together. Given SecondMI, when FirstMI is unspecified, then check if
 /// SecondMI may be part of a fused pair at all.
 static bool shouldScheduleAdjacent(const TargetInstrInfo &TII,
                                    const TargetSubtargetInfo &TSI,
                                    const MachineInstr *FirstMI,
                                    const MachineInstr &SecondMI) {
   const AArch64Subtarget &ST = static_cast<const AArch64Subtarget&>(TSI);
 
   if (ST.hasArithmeticBccFusion() && isArithmeticBccPair(FirstMI, SecondMI))
     return true;
   if (ST.hasArithmeticCbzFusion() && isArithmeticCbzPair(FirstMI, SecondMI))
     return true;
   if (ST.hasFuseAES() && isAESPair(FirstMI, SecondMI))
     return true;
   if (ST.hasFuseLiterals() && isLiteralsPair(FirstMI, SecondMI))
     return true;
   if (ST.hasFuseAddress() && isAddressLdStPair(FirstMI, SecondMI))
     return true;
+  if (ST.hasFuseCCSelect() && isCCSelectPair(FirstMI, SecondMI))
+    return true;
 
   return false;
 }
 
 } // end namespace
 
 
 namespace llvm {
 
 std::unique_ptr<ScheduleDAGMutation> createAArch64MacroFusionDAGMutation () {
   return createMacroFusionDAGMutation(shouldScheduleAdjacent);
 }
 
 } // end namespace llvm

llvm/trunk/lib/Target/AArch64/AArch64Subtarget.h

protected:
   bool Misaligned128StoreIsSlow = false;
   bool Paired128IsSlow = false;
   bool STRQroIsSlow = false;
   bool UseAlternateSExtLoadCVTF32Pattern = false;
   bool HasArithmeticBccFusion = false;
   bool HasArithmeticCbzFusion = false;
   bool HasFuseAddress = false;
   bool HasFuseAES = false;
+  bool HasFuseCCSelect = false;
   bool HasFuseLiterals = false;
   bool DisableLatencySchedHeuristic = false;
   bool UseRSqrt = false;
   uint8_t MaxInterleaveFactor = 2;
   uint8_t VectorInsertExtractBaseCost = 3;
   uint16_t CacheLineSize = 0;
   uint16_t PrefetchDistance = 0;
   uint16_t MinPrefetchStride = 1;
public:
   bool isSTRQroSlow() const { return STRQroIsSlow; }
   bool useAlternateSExtLoadCVTF32Pattern() const {
     return UseAlternateSExtLoadCVTF32Pattern;
   }
   bool hasArithmeticBccFusion() const { return HasArithmeticBccFusion; }
   bool hasArithmeticCbzFusion() const { return HasArithmeticCbzFusion; }
   bool hasFuseAddress() const { return HasFuseAddress; }
   bool hasFuseAES() const { return HasFuseAES; }
+  bool hasFuseCCSelect() const { return HasFuseCCSelect; }
   bool hasFuseLiterals() const { return HasFuseLiterals; }
 
   /// \brief Return true if the CPU supports any kind of instruction fusion.
   bool hasFusion() const {
     return hasArithmeticBccFusion() || hasArithmeticCbzFusion() ||
-           hasFuseAES() || hasFuseLiterals();
+           hasFuseAES() || hasFuseCCSelect() || hasFuseLiterals();
   }
 
   bool useRSqrt() const { return UseRSqrt; }
   unsigned getMaxInterleaveFactor() const { return MaxInterleaveFactor; }
   unsigned getVectorInsertExtractBaseCost() const {
     return VectorInsertExtractBaseCost;
   }
   unsigned getCacheLineSize() const { return CacheLineSize; }

llvm/trunk/test/CodeGen/AArch64/misched-fusion-csel.ll

+; RUN: llc %s -o - -mtriple=aarch64-unknown -mattr=fuse-csel | FileCheck %s
+; RUN: llc %s -o - -mtriple=aarch64-unknown -mcpu=exynos-m3  | FileCheck %s
+
+target triple = "aarch64-unknown"
+
+define i32 @test_sub_cselw(i32 %a0, i32 %a1, i32 %a2) {
+entry:
+  %v0 = sub i32 %a0, 13
+  %cond = icmp eq i32 %v0, 0
+  %v1 = add i32 %a1, 7
+  %v2 = select i1 %cond, i32 %a0, i32 %v1
+  ret i32 %v2
+
+; CHECK-LABEL: test_sub_cselw:
+; CHECK: cmp {{w[0-9]}}, #13
+; CHECK-NEXT: csel {{w[0-9]}}
+}
+
+define i64 @test_sub_cselx(i64 %a0, i64 %a1, i64 %a2) {
+entry:
+  %v0 = sub i64 %a0, 13
+  %cond = icmp eq i64 %v0, 0
+  %v1 = add i64 %a1, 7
+  %v2 = select i1 %cond, i64 %a0, i64 %v1
+  ret i64 %v2
+
+; CHECK-LABEL: test_sub_cselx:
+; CHECK: cmp {{x[0-9]}}, #13
+; CHECK-NEXT: csel {{x[0-9]}}
+}