[globalisel][aarch64] Make getCopyMapping() take register banks ID's rather than IsGPR booleans

[dsanders]

Summary

This allows the function to handle architectures with more than two register banks.

Depends on D27978

[dsanders]

Rebase following changes to the previous patches

[rovka]

lib/Target/AArch64/AArch64GenRegisterBankInfo.def

301–309

You should update the comment.

306

Why is this last member needed? Isn't it invalid to access it in the first place?

[dsanders]

Update comment.
Remove redundant element from BankIDToCopyMapIdx

[dsanders]

Thanks. I've updated the patch

lib/Target/AArch64/AArch64GenRegisterBankInfo.def

306

This is a leftover from before I had the assertions on line 158-159. I'll remove it

[rovka]

Cool, LGTM.

[dsanders]

Moved this patch to the new patch series.

[qcolombet]

Hi Daniel,

I fail to see the goal of this patch.
It does not enable new capabilities nor simplify things.
What am I missing?

Cheers,
-Quentin

[dsanders]

In D27339#628035, @qcolombet wrote:

Hi Daniel,

I fail to see the goal of this patch.
It does not enable new capabilities nor simplify things.
What am I missing?

Cheers,
-Quentin

The aim is to generalize getCopyMapping() so it can support more than two register banks. This will be useful for targets that have special purpose register banks (e.g. the HI/LO registers used by MIPS's multiply and divide and DSP operations, also the co-processor registers), or separate address and data register banks. It should also be useful if AArch64 ever needs to move values between CCR and GPR banks. Later on, I would expect the underlying data tablegen-erated and for getCopyMapping() to be moved to target independent code.

A less important reason (but the one that drew my attention), was that I didn't like the way CHECK_VALUEMAP_CROSSREGCPY() and getCopyMapping() together were converting PartialMappingIdx's to booleans and then back to PartialMappingIdx's and this also caused some problems with my original patch series (CCR needed to be excluded from the copy mapping table).

[dsanders]

Rebased onto new D27978

[dsanders]

Refresh before commit.

lib/Target/AArch64/AArch64GenRegisterBankInfo.def

unsigned ValMappingIdx = First3OpsIdx +
                            (RBIdx - PartialMappingIdx::PMI_Min +
                             getRegBankBaseIdxOffset(RBIdx, Size)) *
                                ValueMappingIdx::DistanceBetweenRegBanks;
   assert(ValMappingIdx >= First3OpsIdx && ValMappingIdx <= Last3OpsIdx &&
          "Mapping out of bound");
 
   return &ValMappings[ValMappingIdx];
 }
 
+AArch64GenRegisterBankInfo::PartialMappingIdx
+    AArch64GenRegisterBankInfo::BankIDToCopyMapIdx[]{
+        PMI_FirstGPR, // GPR
+        PMI_FirstFPR, // FPR
+        PMI_None,     // CCR
+    };
+
 const RegisterBankInfo::ValueMapping *
-AArch64GenRegisterBankInfo::getCopyMapping(bool DstIsGPR, bool SrcIsGPR,
-                                           unsigned Size) {
-  PartialMappingIdx DstRBIdx = DstIsGPR ? PMI_FirstGPR : PMI_FirstFPR;
-  PartialMappingIdx SrcRBIdx = SrcIsGPR ? PMI_FirstGPR : PMI_FirstFPR;
+AArch64GenRegisterBankInfo::getCopyMapping(unsigned DstBankID,
+                                           unsigned SrcBankID, unsigned Size) {
+  assert(DstBankID < AArch64::NumRegisterBanks && "Invalid bank ID");
+  assert(SrcBankID < AArch64::NumRegisterBanks && "Invalid bank ID");
+  PartialMappingIdx DstRBIdx = BankIDToCopyMapIdx[DstBankID];
+  PartialMappingIdx SrcRBIdx = BankIDToCopyMapIdx[SrcBankID];
+  assert(DstRBIdx != PMI_None && "No such mapping");
+  assert(SrcRBIdx != PMI_None && "No such mapping");
+
   if (DstRBIdx == SrcRBIdx)
     return getValueMapping(DstRBIdx, Size);
+
   assert(Size <= 64 && "GPR cannot handle that size");
   unsigned ValMappingIdx =
       FirstCrossRegCpyIdx +
       (DstRBIdx - PMI_Min + getRegBankBaseIdxOffset(DstRBIdx, Size)) *
           ValueMappingIdx::DistanceBetweenCrossRegCpy;
   assert(ValMappingIdx >= FirstCrossRegCpyIdx &&
          ValMappingIdx <= LastCrossRegCpyIdx && "Mapping out of bound");
   return &ValMappings[ValMappingIdx];
 }
 } // End llvm namespace.

lib/Target/AArch64/AArch64RegisterBankInfo.h

 
 class AArch64GenRegisterBankInfo : public RegisterBankInfo {
 private:
   static RegisterBank *RegBanks[];
 
 protected:
   AArch64GenRegisterBankInfo();
 
-public:
-  static RegisterBankInfo::PartialMapping PartMappings[];
-  static RegisterBankInfo::ValueMapping ValMappings[];
-  static bool checkPartialMap(unsigned Idx, unsigned ValStartIdx,
-                              unsigned ValLength, const RegisterBank &RB);
-  static bool checkValueMapImpl(unsigned Idx, unsigned FirstInBank,
-                                unsigned Size, unsigned Offset);
   enum PartialMappingIdx {
     PMI_None = -1,
     PMI_GPR32 = 1,
     PMI_GPR64,
     PMI_FPR32,
     PMI_FPR64,
     PMI_FPR128,
     PMI_FPR256,
     PMI_FPR512,
     PMI_FirstGPR = PMI_GPR32,
     PMI_LastGPR = PMI_GPR64,
     PMI_FirstFPR = PMI_FPR32,
     PMI_LastFPR = PMI_FPR512,
     PMI_Min = PMI_FirstGPR,
   };
 
+  static RegisterBankInfo::PartialMapping PartMappings[];
+  static RegisterBankInfo::ValueMapping ValMappings[];
+  static PartialMappingIdx BankIDToCopyMapIdx[];
+
   enum ValueMappingIdx {
     First3OpsIdx = 0,
     Last3OpsIdx = 18,
     DistanceBetweenRegBanks = 3,
     FirstCrossRegCpyIdx = 21,
     LastCrossRegCpyIdx = 27,
     DistanceBetweenCrossRegCpy = 2
   };
 
+  static bool checkPartialMap(unsigned Idx, unsigned ValStartIdx,
+                              unsigned ValLength, const RegisterBank &RB);
+  static bool checkValueMapImpl(unsigned Idx, unsigned FirstInBank,
+                                unsigned Size, unsigned Offset);
   static bool checkPartialMappingIdx(PartialMappingIdx FirstAlias,
                                      PartialMappingIdx LastAlias,
                                      std::vector<PartialMappingIdx> Order);
 
   static unsigned getRegBankBaseIdxOffset(unsigned RBIdx, unsigned Size);
 
   /// Get the pointer to the ValueMapping representing the RegisterBank
   /// at \p RBIdx with a size of \p Size.
   ///
   /// The returned mapping works for instructions with the same kind of
   /// operands for up to 3 operands.
   ///
   /// \pre \p RBIdx != PartialMappingIdx::None
   static const RegisterBankInfo::ValueMapping *
   getValueMapping(PartialMappingIdx RBIdx, unsigned Size);
 
   /// Get the pointer to the ValueMapping of the operands of a copy
-  /// instruction from a GPR or FPR register to a GPR or FPR register
-  /// with a size of \p Size.
-  ///
-  /// If \p DstIsGPR is true, the destination of the copy is on GPR,
-  /// otherwise it is on FPR. Same thing for \p SrcIsGPR.
+  /// instruction from the \p SrcBankID register bank to the \p DstBankID
+  /// register bank with a size of \p Size.
   static const RegisterBankInfo::ValueMapping *
-  getCopyMapping(bool DstIsGPR, bool SrcIsGPR, unsigned Size);
+  getCopyMapping(unsigned DstBankID, unsigned SrcBankID, unsigned Size);
 };
 
 /// This class provides the information for the target register banks.
 class AArch64RegisterBankInfo final : public AArch64GenRegisterBankInfo {
   /// See RegisterBankInfo::applyMapping.
   void applyMappingImpl(const OperandsMapper &OpdMapper) const override;
 
   /// Get an instruction mapping where all the operands map to

lib/Target/AArch64/AArch64RegisterBankInfo.cpp

#define CHECK_VALUEMAP_3OPS(RBName, Size)                                      \
   CHECK_VALUEMAP_3OPS(FPR, 512);
 
 #define CHECK_VALUEMAP_CROSSREGCPY(RBNameDst, RBNameSrc, Size)                 \
   do {                                                                         \
     unsigned PartialMapDstIdx = PMI_##RBNameDst##Size - PMI_Min;               \
     unsigned PartialMapSrcIdx = PMI_##RBNameSrc##Size - PMI_Min;               \
     (void)PartialMapDstIdx;                                                    \
     (void)PartialMapSrcIdx;                                                    \
-    const ValueMapping *Map =                                                  \
-        getCopyMapping(PMI_First##RBNameDst == PMI_FirstGPR,                   \
-                       PMI_First##RBNameSrc == PMI_FirstGPR, Size);            \
+    const ValueMapping *Map = getCopyMapping(                                  \
+        AArch64::RBNameDst##RegBankID, AArch64::RBNameSrc##RegBankID, Size);  \
     (void)Map;                                                                 \
     assert(Map[0].BreakDown ==                                                 \
                &AArch64GenRegisterBankInfo::PartMappings[PartialMapDstIdx] &&  \
            Map[0].NumBreakDowns == 1 && #RBNameDst #Size                       \
            " Dst is incorrectly initialized");                                 \
     assert(Map[1].BreakDown ==                                                 \
                &AArch64GenRegisterBankInfo::PartMappings[PartialMapSrcIdx] &&  \
            Map[1].NumBreakDowns == 1 && #RBNameSrc #Size                       \
case TargetOpcode::G_BITCAST: {
     // If the instruction has any implicit-defs or uses,
     // do not mess with it.
     if (MI.getNumOperands() != 2)
       break;
 
     InstructionMappings AltMappings;
     InstructionMapping GPRMapping(
         /*ID*/ 1, /*Cost*/ 1,
-        getCopyMapping(/*DstIsGPR*/ true, /*SrcIsGPR*/ true, Size),
+        getCopyMapping(AArch64::GPRRegBankID, AArch64::GPRRegBankID, Size),
         /*NumOperands*/ 2);
     InstructionMapping FPRMapping(
         /*ID*/ 2, /*Cost*/ 1,
-        getCopyMapping(/*DstIsGPR*/ false, /*SrcIsGPR*/ false, Size),
+        getCopyMapping(AArch64::FPRRegBankID, AArch64::FPRRegBankID, Size),
         /*NumOperands*/ 2);
     InstructionMapping GPRToFPRMapping(
         /*ID*/ 3,
         /*Cost*/ copyCost(AArch64::GPRRegBank, AArch64::FPRRegBank, Size),
-        getCopyMapping(/*DstIsGPR*/ false, /*SrcIsGPR*/ true, Size),
+        getCopyMapping(AArch64::FPRRegBankID, AArch64::GPRRegBankID, Size),
         /*NumOperands*/ 2);
     InstructionMapping FPRToGPRMapping(
         /*ID*/ 3,
         /*Cost*/ copyCost(AArch64::GPRRegBank, AArch64::FPRRegBank, Size),
-        getCopyMapping(/*DstIsGPR*/ true, /*SrcIsGPR*/ false, Size),
+        getCopyMapping(AArch64::GPRRegBankID, AArch64::FPRRegBankID, Size),
         /*NumOperands*/ 2);
 
     AltMappings.emplace_back(std::move(GPRMapping));
     AltMappings.emplace_back(std::move(FPRMapping));
     AltMappings.emplace_back(std::move(GPRToFPRMapping));
     AltMappings.emplace_back(std::move(FPRToGPRMapping));
     return AltMappings;
   }
case TargetOpcode::G_BITCAST: {
     LLT SrcTy = MRI.getType(MI.getOperand(1).getReg());
     unsigned Size = DstTy.getSizeInBits();
     bool DstIsGPR = !DstTy.isVector();
     bool SrcIsGPR = !SrcTy.isVector();
     const RegisterBank &DstRB =
         DstIsGPR ? AArch64::GPRRegBank : AArch64::FPRRegBank;
     const RegisterBank &SrcRB =
         SrcIsGPR ? AArch64::GPRRegBank : AArch64::FPRRegBank;
-    return InstructionMapping{DefaultMappingID, copyCost(DstRB, SrcRB, Size),
-                              getCopyMapping(DstIsGPR, SrcIsGPR, Size),
-                              /*NumOperands*/ 2};
+    return InstructionMapping{
+        DefaultMappingID, copyCost(DstRB, SrcRB, Size),
+        getCopyMapping(DstRB.getID(), SrcRB.getID(), Size),
   }
   case TargetOpcode::G_SEQUENCE:
     // FIXME: support this, but the generic code is really not going to do
     // anything sane.
     return InstructionMapping();
   default:
     break;
   }