[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

164

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

164

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

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

234

Same as above

251

same as above

256

same as above

[evandro]

llvm/lib/Target/AArch64/AArch64MacroFusion.cpp

230

Of course!

llvm/lib/Target/AArch64/AArch64MacroFusion.cpp

static bool isLiteralsPair(const MachineInstr *FirstMI,
   // Lower half of 64 bit immediate.
   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.
   if ((FirstOpcode == AArch64::INSTRUCTION_LIST_END ||
        (FirstOpcode == AArch64::MOVKXi &&
         FirstMI->getOperand(3).getImm() == 32)) &&
       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,
case AArch64::ADR:
       return false;
     case AArch64::ADRP:
       return true;
     }
   }
   return false;
 }
 
+// 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:
+        if (AArch64InstrInfo::hasShiftedReg(*FirstMI))
+          return false;
+        return true;
+      case AArch64::SUBSWrx:
+        if (AArch64InstrInfo::hasExtendedReg(*FirstMI))
+          return false;
+        return true;
+      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:
+        if (AArch64InstrInfo::hasShiftedReg(*FirstMI))
+          return false;
+        return true;
+      case AArch64::SUBSXrx:
+      case AArch64::SUBSXrx64:
+        if (AArch64InstrInfo::hasExtendedReg(*FirstMI))
+          return false;
+        return true;
+      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