[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

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

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

130

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

134

Same as above

151

same as above

156

same as above

[evandro]

llvm/lib/Target/AArch64/AArch64MacroFusion.cpp

130

Of course!

llvm/lib/Target/AArch64/AArch64.td

 def FeatureArithmeticCbzFusion : SubtargetFeature<
     "arith-cbz-fusion", "HasArithmeticCbzFusion", "true",
     "CPU fuses arithmetic + cbz/cbnz 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,
                                      FeatureCustomCheapAsMoveHandling,
                                      FeatureFPARMv8,
                                      FeatureFuseAES,
+                                     FeatureFuseCCSelect,
                                      FeatureFuseLiterals,
                                      FeatureNEON,
                                      FeaturePerfMon,
                                      FeaturePostRAScheduler,
                                      FeatureZCZeroing]>;
 
 def ProcKryo    : SubtargetFeature<"kryo", "ARMProcFamily", "Kryo",
                                    "Qualcomm Kryo processors", [

llvm/lib/Target/AArch64/AArch64MacroFusion.cpp

unsigned FirstOpcode =
       FirstMI ? FirstMI->getOpcode()
               : static_cast<unsigned>(AArch64::INSTRUCTION_LIST_END);
   unsigned SecondOpcode = SecondMI.getOpcode();
 
   if (ST.hasArithmeticBccFusion())
     // Fuse CMN, CMP, TST followed by Bcc.
     if (SecondOpcode == AArch64::Bcc)
       switch (FirstOpcode) {
-      default:
-        return false;
+      case AArch64::INSTRUCTION_LIST_END:
+        return true;
       case AArch64::ADDSWri:
       case AArch64::ADDSWrr:
       case AArch64::ADDSXri:
       case AArch64::ADDSXrr:
       case AArch64::ANDSWri:
       case AArch64::ANDSWrr:
       case AArch64::ANDSXri:
       case AArch64::ANDSXrr:
       case AArch64::SUBSWri:
       case AArch64::SUBSWrr:
       case AArch64::SUBSXri:
       case AArch64::SUBSXrr:
       case AArch64::BICSWrr:
       case AArch64::BICSXrr:
         return true;
       case AArch64::ADDSWrs:
       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...
-        return !II.hasShiftedReg(*FirstMI);
-      case AArch64::INSTRUCTION_LIST_END:
+        if (!II.hasShiftedReg(*FirstMI))
           return true;
+        break;
       }
 
   if (ST.hasArithmeticCbzFusion())
     // Fuse ALU operations followed by CBZ/CBNZ.
     if (SecondOpcode == AArch64::CBNZW || SecondOpcode == AArch64::CBNZX ||
         SecondOpcode == AArch64::CBZW || SecondOpcode == AArch64::CBZX)
       switch (FirstOpcode) {
-      default:
-        return false;
+      case AArch64::INSTRUCTION_LIST_END:
+        return true;
       case AArch64::ADDWri:
       case AArch64::ADDWrr:
       case AArch64::ADDXri:
       case AArch64::ADDXrr:
       case AArch64::ANDWri:
       case AArch64::ANDWrr:
       case AArch64::ANDXri:
       case AArch64::ANDXrr:
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...
-        return !II.hasShiftedReg(*FirstMI);
-      case AArch64::INSTRUCTION_LIST_END:
+        if (!II.hasShiftedReg(*FirstMI))
           return true;
+        break;
       }
 
-  if (ST.hasFuseAES())
+  if (ST.hasFuseAES()) {
     // Fuse AES crypto operations.
-    switch(SecondOpcode) {
     // AES encode.
-    case AArch64::AESMCrr:
-    case AArch64::AESMCrrTied:
-      return FirstOpcode == AArch64::AESErr ||
-             FirstOpcode == AArch64::INSTRUCTION_LIST_END;
+    if ((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
+         FirstOpcode == AArch64::AESErr) &&
+        (SecondOpcode == AArch64::AESMCrr ||
+         SecondOpcode == AArch64::AESMCrrTied))
+      return true;
     // AES decode.
-    case AArch64::AESIMCrr:
-    case AArch64::AESIMCrrTied:
-      return FirstOpcode == AArch64::AESDrr ||
-             FirstOpcode == AArch64::INSTRUCTION_LIST_END;
-    }
+    else if ((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
+              FirstOpcode == AArch64::AESDrr) &&
+             (SecondOpcode == AArch64::AESIMCrr ||
+              SecondOpcode == AArch64::AESIMCrrTied))
+      return true;
 
-  if (ST.hasFuseLiterals())
+  if (ST.hasFuseLiterals()) {
     // Fuse literal generation operations.
-    switch (SecondOpcode) {
     // PC relative address.
-    case AArch64::ADDXri:
-      return FirstOpcode == AArch64::ADRP ||
-             FirstOpcode == AArch64::INSTRUCTION_LIST_END;
+    if ((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
+         FirstOpcode == AArch64::ADRP) &&
+        SecondOpcode == AArch64::ADDXri)
+      return true;
     // 32 bit immediate.
-    case AArch64::MOVKWi:
-      return (FirstOpcode == AArch64::MOVZWi &&
-              SecondMI.getOperand(3).getImm() == 16) ||
-             FirstOpcode == AArch64::INSTRUCTION_LIST_END;
-    // Lower and upper half of 64 bit immediate.
-    case AArch64::MOVKXi:
-      return FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
-             (FirstOpcode == AArch64::MOVZXi &&
-              SecondMI.getOperand(3).getImm() == 16) ||
+    else if ((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
+              FirstOpcode == AArch64::MOVZWi) &&
+             (SecondOpcode == AArch64::MOVKWi &&
+              SecondMI.getOperand(3).getImm() == 16))
+      return true;
+    // Lower half of 64 bit immediate.
+    else if((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
+             FirstOpcode == AArch64::MOVZXi) &&
+            (SecondOpcode == AArch64::MOVKXi &&
+             SecondMI.getOperand(3).getImm() == 16))
+      return true;
+    // Upper half of 64 bit immediate.
+    else if ((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
               (FirstOpcode == AArch64::MOVKXi &&
-              FirstMI->getOperand(3).getImm() == 32 &&
-              SecondMI.getOperand(3).getImm() == 48);
+               FirstMI->getOperand(3).getImm() == 32)) &&
+             (SecondOpcode == AArch64::MOVKXi &&
+              SecondMI.getOperand(3).getImm() == 48))
+      return true;
+  }
+
+  if (ST.hasFuseCCSelect()) {
+    // Fuse conditional select operations.
+    if (SecondOpcode == AArch64::CSELWr)
+      // 32 bits
+      switch (FirstOpcode) {
+      case AArch64::INSTRUCTION_LIST_END:
+        return true;
+      case AArch64::SUBSWrs:
+        if (II.hasShiftedReg(*FirstMI))
+          break;
+        // Fall through.
+      case AArch64::SUBSWrx:
+        if (II.hasExtendedReg(*FirstMI))
+          break;
+        // Fall through.
+      case AArch64::SUBSWrr:
+      case AArch64::SUBSWri:
+        if (FirstMI->definesRegister(AArch64::WZR))
+          return true;
+        break;
+      }
+    else if (SecondOpcode == AArch64::CSELXr)
+      // 64 bits
+      switch (FirstOpcode) {
+      case AArch64::INSTRUCTION_LIST_END:
+        return true;
+      case AArch64::SUBSXrs:
+        if (II.hasShiftedReg(*FirstMI))
+          break;
+        // Fall through.
+      case AArch64::SUBSXrx:
+      case AArch64::SUBSXrx64:
+        if (II.hasExtendedReg(*FirstMI))
+          break;
+        // Fall through.
+      case AArch64::SUBSXrr:
+      case AArch64::SUBSXri:
+        if (FirstMI->definesRegister(AArch64::XZR))
+          return true;
+        break;
+      }
     }
 
   return false;
 }
 
 } // end namespace
 
 
 namespace llvm {
 
 std::unique_ptr<ScheduleDAGMutation> createAArch64MacroFusionDAGMutation () {
   return createMacroFusionDAGMutation(shouldScheduleAdjacent);
 }
 
 } // end namespace llvm

llvm/lib/Target/AArch64/AArch64Subtarget.h

protected:
   bool UsePostRAScheduler = false;
   bool Misaligned128StoreIsSlow = false;
   bool Paired128IsSlow = false;
   bool STRQroIsSlow = false;
   bool UseAlternateSExtLoadCVTF32Pattern = false;
   bool HasArithmeticBccFusion = false;
   bool HasArithmeticCbzFusion = 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 isPaired128Slow() const { return Paired128IsSlow; }
   bool isSTRQroSlow() const { return STRQroIsSlow; }
   bool useAlternateSExtLoadCVTF32Pattern() const {
     return UseAlternateSExtLoadCVTF32Pattern;
   }
   bool hasArithmeticBccFusion() const { return HasArithmeticBccFusion; }
   bool hasArithmeticCbzFusion() const { return HasArithmeticCbzFusion; }
   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();
   }
 

llvm/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]}}
+}