[BOLT][NFC] Fix MCPlusBuilder::getAliases caching behavior

[Amir]

Summary

Caching behavior of getAliases causes a failure in unit tests where two
MCPlusBuilder objects are created corresponding to AArch64 and X86:
the alias cache is created for AArch64 but then used for X86.

https://lab.llvm.org/staging/#/builders/211/builds/126

The issue only affects unit tests as we only construct one MCPlusBuilder
for ELF binary.

Resolve the issue by moving alias bitvectors to MCPlusBuilder object.

[yota9]

-

bolt/lib/Core/MCPlusBuilder.cpp

443–444

Thanks, strange that we didn't come across it before. But maybe it would be better to move these vectors to MCPlusBuilder object?

[Amir]

bolt/lib/Core/MCPlusBuilder.cpp

443–444

Yes, it’s really strange, but I've confirmed on a buildbot that it's really happening. Looks like we might randomly "get lucky" with aliases depending on register ids.

It's a good idea to move these vectors to MCPlusBuilder.

[Amir]

Move AliasMap and SmallerAliasMap into MCPlusBuilder object;
split initAliases from getAliases.

[yota9]

bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp

1153 ↗

(On Diff #427072)

Maybe move it to the common MCPlusBuilder constructor?

[Amir]

Move initAliases to MCPlusBuilder ctor

[Amir]

bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp

1153 ↗

(On Diff #427072)

That works too. Wasn't sure about invoking a virtual method from a base constructor.

[yota9]

LGTM Thanks!

bolt/include/bolt/Core/MCPlusBuilder.h

 
 public:
   MCPlusBuilder(const MCInstrAnalysis *Analysis, const MCInstrInfo *Info,
                 const MCRegisterInfo *RegInfo)
       : Analysis(Analysis), Info(Info), RegInfo(RegInfo) {
     // Initialize the default annotation allocator with id 0
     AnnotationAllocators.emplace(0, AnnotationAllocator());
     MaxAllocatorId++;
+    // Build alias map
+    initAliases();
   }
 
   /// Initialize a new annotation allocator and return its id
   AllocatorIdTy initializeNewAnnotationAllocator() {
     AnnotationAllocators.emplace(MaxAllocatorId, AnnotationAllocator());
     return MaxAllocatorId++;
   }
 
virtual const MCExpr *getTargetExprFor(MCInst &Inst, const MCExpr *Expr,
     return Expr;
   }
 
   /// Return a BitVector marking all sub or super registers of \p Reg, including
   /// itself.
   virtual const BitVector &getAliases(MCPhysReg Reg,
                                       bool OnlySmaller = false) const;
 
+  /// Initialize aliases tables.
+  virtual void initAliases();
+
   /// Change \p Regs setting all registers used to pass parameters according
   /// to the host abi. Do nothing if not implemented.
   virtual BitVector getRegsUsedAsParams() const {
     llvm_unreachable("not implemented");
     return BitVector();
   }
 
   /// Change \p Regs setting all registers used as callee-saved according

 
   virtual BlocksVectorTy jumpTablePromotion(
       const MCInst &IJmpInst,
       const std::vector<std::pair<MCSymbol *, uint64_t>> &Targets,
       const std::vector<MCInst *> &TargetFetchInsns, MCContext *Ctx) const {
     llvm_unreachable("not implemented");
     return BlocksVectorTy();
   }
+
+  // AliasMap caches a mapping of registers to the set of registers that
+  // alias (are sub or superregs of itself, including itself).
+  std::vector<BitVector> AliasMap;
+  std::vector<BitVector> SmallerAliasMap;
 };
 
 MCPlusBuilder *createX86MCPlusBuilder(const MCInstrAnalysis *,
                                       const MCInstrInfo *,
                                       const MCRegisterInfo *);
 
 MCPlusBuilder *createAArch64MCPlusBuilder(const MCInstrAnalysis *,
                                           const MCInstrInfo *,
                                           const MCRegisterInfo *);
 
 } // namespace bolt
 } // namespace llvm
 
 #endif

bolt/lib/Core/MCPlusBuilder.cpp

if (const uint16_t *ImpUses = InstInfo.ImplicitUses) {
     for (; *ImpUses; ++ImpUses)
       if (*ImpUses == Reg || RegInfo->isSubRegister(Reg, *ImpUses))
         return true;
   }
   return false;
 }
 
 const BitVector &MCPlusBuilder::getAliases(MCPhysReg Reg,
                                            bool OnlySmaller) const {
-  // AliasMap caches a mapping of registers to the set of registers that
-  // alias (are sub or superregs of itself, including itself).
-  static std::vector<BitVector> AliasMap;
-  static std::vector<BitVector> SmallerAliasMap;
-
-  if (AliasMap.size() > 0) {
-    if (OnlySmaller)
-      return SmallerAliasMap[Reg];
-    return AliasMap[Reg];
-  }
 
+void MCPlusBuilder::initAliases() {
+  assert(AliasMap.size() == 0 && SmallerAliasMap.size() == 0);
   // Build alias map
   for (MCPhysReg I = 0, E = RegInfo->getNumRegs(); I != E; ++I) {
     BitVector BV(RegInfo->getNumRegs(), false);
     BV.set(I);
     AliasMap.emplace_back(BV);
     SmallerAliasMap.emplace_back(BV);
   }
 
for (MCPhysReg I = 0, E = RegInfo->getNumRegs(); I != E; ++I) {
       int Idx = BV.find_first();
       while (Idx != -1) {
         dbgs() << Idx << " ";
         Idx = BV.find_next(Idx);
       }
       dbgs() << "\n";
     }
   });
-
-  if (OnlySmaller)
-    return SmallerAliasMap[Reg];
-  return AliasMap[Reg];
 }
 
 uint8_t MCPlusBuilder::getRegSize(MCPhysReg Reg) const {
   // SizeMap caches a mapping of registers to their sizes
   static std::vector<uint8_t> SizeMap;
 
   if (SizeMap.size() > 0) {
     return SizeMap[Reg];