[BOLT] Modify MCPlus annotation internals. NFCI. (#70412)

When annotating MCInst instructions, attach extra annotation operands
directly to the annotated instruction, instead of attaching them to an
instruction pointed to by a special kInst operand.

With this change, it's no longer necessary to allocate MCInst and most
of the first-class annotations come with free memory as currently MCInst
is declared with:

    SmallVector<MCOperand, 10> Operands;

i.e. more operands than are normally being used.

We still create a kInst operand with a nullptr instruction value to
designate the beginning of annotation operands. However, this special
operand might not be needed if we can rely on MCInstrDesc::NumOperands.

Author: maksfb

bolt/include/bolt/Core/MCPlus.h

@@ -32,11 +32,16 @@ namespace MCPlus {
 /// pad and the uint64_t represents the action.
 using MCLandingPad = std::pair<const MCSymbol *, uint64_t>;
 
-/// An extension to MCInst is provided via an extra operand of type MCInst with
-/// ANNOTATION_LABEL opcode (i.e. we are tying an annotation instruction to an
-/// existing one). The annotation instruction contains a list of Immediate
-/// operands. Each operand either contains a value, or is a pointer to
-/// an instance of class MCAnnotation.
+/// An extension to MCInst is provided via extra operands, i.e. operands that
+/// are not used in the instruction assembly. Any kind of metadata can be
+/// attached to MCInst with this "annotation" extension using MCPlusBuilder
+/// interface.
+//
+/// The first extra operand must be of type kInst with an empty (nullptr)
+/// value. The kInst operand type is unused on most non-VLIW architectures.
+/// We use it to mark the beginning of annotations operands. The rest of the
+/// operands are of Immediate type with annotation info encoded into the value
+/// of the immediate.
 ///
 /// There are 2 distinct groups of annotations. The first group is a first-class
 /// annotation that affects semantics of the instruction, such as an
@@ -55,7 +60,7 @@ using MCLandingPad = std::pair<const MCSymbol *, uint64_t>;
 /// of their corresponding operand.
 ///
 /// Annotations in the second group could be addressed either by name, or by
-/// by and index which could be queried by providing a name.
+/// by index which could be queried by providing the name.
 class MCAnnotation {
 public:
   enum Kind {
@@ -106,10 +111,11 @@ template <typename ValueType> class MCSimpleAnnotation : public MCAnnotation {
 /// Return a number of operands in \Inst excluding operands representing
 /// annotations.
 inline unsigned getNumPrimeOperands(const MCInst &Inst) {
-  if (Inst.getNumOperands() > 0 && std::prev(Inst.end())->isInst()) {
-    assert(std::prev(Inst.end())->getInst()->getOpcode() ==
-           TargetOpcode::ANNOTATION_LABEL);
-    return Inst.getNumOperands() - 1;
+  for (signed I = Inst.getNumOperands() - 1; I >= 0; --I) {
+    if (Inst.getOperand(I).isInst())
+      return I;
+    if (!Inst.getOperand(I).isImm())
+      return Inst.getNumOperands();
   }
   return Inst.getNumOperands();
 }

bolt/include/bolt/Core/MCPlusBuilder.h

@@ -65,7 +65,6 @@ class MCPlusBuilder {
 private:
   /// A struct that represents a single annotation allocator
   struct AnnotationAllocator {
-    SpecificBumpPtrAllocator<MCInst> MCInstAllocator;
     BumpPtrAllocator ValueAllocator;
     std::unordered_set<MCPlus::MCAnnotation *> AnnotationPool;
   };
@@ -97,60 +96,62 @@ class MCPlusBuilder {
     return SignExtend64<56>(ImmValue & 0xff'ffff'ffff'ffffULL);
   }
 
-  MCInst *getAnnotationInst(const MCInst &Inst) const {
-    if (Inst.getNumOperands() == 0)
-      return nullptr;
+  std::optional<unsigned> getFirstAnnotationOpIndex(const MCInst &Inst) const {
+    const unsigned NumPrimeOperands = MCPlus::getNumPrimeOperands(Inst);
+    if (Inst.getNumOperands() == NumPrimeOperands)
+      return std::nullopt;
 
-    const MCOperand &LastOp = Inst.getOperand(Inst.getNumOperands() - 1);
-    if (!LastOp.isInst())
-      return nullptr;
+    assert(Inst.getOperand(NumPrimeOperands).getInst() == nullptr &&
+           "Empty instruction expected.");
 
-    MCInst *AnnotationInst = const_cast<MCInst *>(LastOp.getInst());
-    assert(AnnotationInst->getOpcode() == TargetOpcode::ANNOTATION_LABEL);
+    return NumPrimeOperands + 1;
+  }
 
-    return AnnotationInst;
+  MCInst::iterator getAnnotationInstOp(MCInst &Inst) const {
+    for (MCInst::iterator Iter = Inst.begin(); Iter != Inst.end(); ++Iter) {
+      if (Iter->isInst()) {
+        assert(Iter->getInst() == nullptr && "Empty instruction expected.");
+        return Iter;
+      }
+    }
+    return Inst.end();
   }
 
-  void removeAnnotationInst(MCInst &Inst) const {
-    assert(getAnnotationInst(Inst) && "Expected annotation instruction.");
-    Inst.erase(std::prev(Inst.end()));
-    assert(!getAnnotationInst(Inst) &&
-           "More than one annotation instruction detected.");
+  void removeAnnotations(MCInst &Inst) const {
+    Inst.erase(getAnnotationInstOp(Inst), Inst.end());
   }
 
-  void setAnnotationOpValue(MCInst &Inst, unsigned Index, int64_t Value,
-                            AllocatorIdTy AllocatorId = 0) {
-    MCInst *AnnotationInst = getAnnotationInst(Inst);
-    if (!AnnotationInst) {
-      AnnotationAllocator &Allocator = getAnnotationAllocator(AllocatorId);
-      AnnotationInst = new (Allocator.MCInstAllocator.Allocate()) MCInst();
-      AnnotationInst->setOpcode(TargetOpcode::ANNOTATION_LABEL);
-      Inst.addOperand(MCOperand::createInst(AnnotationInst));
+  void setAnnotationOpValue(MCInst &Inst, unsigned Index, int64_t Value) const {
+    const int64_t AnnotationValue = encodeAnnotationImm(Index, Value);
+    const std::optional<unsigned> FirstAnnotationOp =
+        getFirstAnnotationOpIndex(Inst);
+    if (!FirstAnnotationOp) {
+      Inst.addOperand(MCOperand::createInst(nullptr));
+      Inst.addOperand(MCOperand::createImm(AnnotationValue));
+      return;
     }
 
-    const int64_t AnnotationValue = encodeAnnotationImm(Index, Value);
-    for (int I = AnnotationInst->getNumOperands() - 1; I >= 0; --I) {
-      int64_t ImmValue = AnnotationInst->getOperand(I).getImm();
+    for (unsigned I = *FirstAnnotationOp; I < Inst.getNumOperands(); ++I) {
+      const int64_t ImmValue = Inst.getOperand(I).getImm();
       if (extractAnnotationIndex(ImmValue) == Index) {
-        AnnotationInst->getOperand(I).setImm(AnnotationValue);
+        Inst.getOperand(I).setImm(AnnotationValue);
         return;
       }
     }
 
-    AnnotationInst->addOperand(MCOperand::createImm(AnnotationValue));
+    Inst.addOperand(MCOperand::createImm(AnnotationValue));
   }
 
   std::optional<int64_t> getAnnotationOpValue(const MCInst &Inst,
                                               unsigned Index) const {
-    const MCInst *AnnotationInst = getAnnotationInst(Inst);
-    if (!AnnotationInst)
+    std::optional<unsigned> FirstAnnotationOp = getFirstAnnotationOpIndex(Inst);
+    if (!FirstAnnotationOp)
       return std::nullopt;
 
-    for (int I = AnnotationInst->getNumOperands() - 1; I >= 0; --I) {
-      int64_t ImmValue = AnnotationInst->getOperand(I).getImm();
-      if (extractAnnotationIndex(ImmValue) == Index) {
+    for (unsigned I = *FirstAnnotationOp; I < Inst.getNumOperands(); ++I) {
+      const int64_t ImmValue = Inst.getOperand(I).getImm();
+      if (extractAnnotationIndex(ImmValue) == Index)
         return extractAnnotationValue(ImmValue);
-      }
     }
 
     return std::nullopt;
@@ -172,21 +173,18 @@ class MCPlusBuilder {
   /// AnnotationNameIndexMap and AnnotationsNames.
   mutable llvm::sys::RWMutex AnnotationNameMutex;
 
-  /// Allocate the TailCall annotation value. Clients of the target-specific
+  /// Set TailCall annotation value to true. Clients of the target-specific
   /// MCPlusBuilder classes must use convert/lower/create* interfaces instead.
-  void setTailCall(MCInst &Inst);
+  void setTailCall(MCInst &Inst) const;
 
 public:
   /// Transfer annotations from \p SrcInst to \p DstInst.
   void moveAnnotations(MCInst &&SrcInst, MCInst &DstInst) const {
-    assert(!getAnnotationInst(DstInst) &&
-           "Destination instruction should not have annotations.");
-    const MCInst *AnnotationInst = getAnnotationInst(SrcInst);
-    if (!AnnotationInst)
-      return;
+    MCInst::iterator AnnotationOp = getAnnotationInstOp(SrcInst);
+    for (MCInst::iterator Iter = AnnotationOp; Iter != SrcInst.end(); ++Iter)
+      DstInst.addOperand(*Iter);
 
-    DstInst.addOperand(MCOperand::createInst(AnnotationInst));
-    removeAnnotationInst(SrcInst);
+    SrcInst.erase(AnnotationOp, SrcInst.end());
   }
 
   /// Return iterator range covering def operands.
@@ -390,7 +388,6 @@ class MCPlusBuilder {
 
       Allocator.AnnotationPool.clear();
       Allocator.ValueAllocator.Reset();
-      Allocator.MCInstAllocator.DestroyAll();
     }
   }
 
@@ -1128,20 +1125,19 @@ class MCPlusBuilder {
   std::optional<MCPlus::MCLandingPad> getEHInfo(const MCInst &Inst) const;
 
   /// Add handler and action info for call instruction.
-  void addEHInfo(MCInst &Inst, const MCPlus::MCLandingPad &LP);
+  void addEHInfo(MCInst &Inst, const MCPlus::MCLandingPad &LP) const;
 
   /// Update exception-handling info for the invoke instruction \p Inst.
   /// Return true on success and false otherwise, e.g. if the instruction is
   /// not an invoke.
-  bool updateEHInfo(MCInst &Inst, const MCPlus::MCLandingPad &LP);
+  bool updateEHInfo(MCInst &Inst, const MCPlus::MCLandingPad &LP) const;
 
   /// Return non-negative GNU_args_size associated with the instruction
   /// or -1 if there's no associated info.
   int64_t getGnuArgsSize(const MCInst &Inst) const;
 
   /// Add the value of GNU_args_size to Inst if it already has EH info.
-  void addGnuArgsSize(MCInst &Inst, int64_t GnuArgsSize,
-                      AllocatorIdTy AllocId = 0);
+  void addGnuArgsSize(MCInst &Inst, int64_t GnuArgsSize) const;
 
   /// Return jump table addressed by this instruction.
   uint64_t getJumpTable(const MCInst &Inst) const;
@@ -1154,19 +1150,19 @@ class MCPlusBuilder {
                     AllocatorIdTy AllocId = 0);
 
   /// Disassociate instruction with a jump table.
-  bool unsetJumpTable(MCInst &Inst);
+  bool unsetJumpTable(MCInst &Inst) const;
 
   /// Return destination of conditional tail call instruction if \p Inst is one.
   std::optional<uint64_t> getConditionalTailCall(const MCInst &Inst) const;
 
   /// Mark the \p Instruction as a conditional tail call, and set its
   /// destination address if it is known. If \p Instruction was already marked,
   /// update its destination with \p Dest.
-  bool setConditionalTailCall(MCInst &Inst, uint64_t Dest = 0);
+  bool setConditionalTailCall(MCInst &Inst, uint64_t Dest = 0) const;
 
   /// If \p Inst was marked as a conditional tail call convert it to a regular
   /// branch. Return true if the instruction was converted.
-  bool unsetConditionalTailCall(MCInst &Inst);
+  bool unsetConditionalTailCall(MCInst &Inst) const;
 
   /// Return offset of \p Inst in the original function, if available.
   std::optional<uint32_t> getOffset(const MCInst &Inst) const;
@@ -1175,10 +1171,10 @@ class MCPlusBuilder {
   uint32_t getOffsetWithDefault(const MCInst &Inst, uint32_t Default) const;
 
   /// Set offset of \p Inst in the original function.
-  bool setOffset(MCInst &Inst, uint32_t Offset, AllocatorIdTy AllocatorId = 0);
+  bool setOffset(MCInst &Inst, uint32_t Offset) const;
 
   /// Remove offset annotation.
-  bool clearOffset(MCInst &Inst);
+  bool clearOffset(MCInst &Inst) const;
 
   /// Return the label of \p Inst, if available.
   MCSymbol *getLabel(const MCInst &Inst) const;
@@ -1827,8 +1823,7 @@ class MCPlusBuilder {
 
     if (!std::is_trivial<ValueType>::value)
       Allocator.AnnotationPool.insert(A);
-    setAnnotationOpValue(Inst, Index, reinterpret_cast<int64_t>(A),
-                         AllocatorId);
+    setAnnotationOpValue(Inst, Index, reinterpret_cast<int64_t>(A));
     return A->getValue();
   }
 
@@ -1961,21 +1956,21 @@ class MCPlusBuilder {
   ///
   /// Return true if the annotation was removed, false if the annotation
   /// was not present.
-  bool removeAnnotation(MCInst &Inst, unsigned Index);
+  bool removeAnnotation(MCInst &Inst, unsigned Index) const;
 
   /// Remove annotation associated with \p Name.
   ///
   /// Return true if the annotation was removed, false if the annotation
   /// was not present.
-  bool removeAnnotation(MCInst &Inst, StringRef Name) {
+  bool removeAnnotation(MCInst &Inst, StringRef Name) const {
     const auto Index = getAnnotationIndex(Name);
     if (!Index)
       return false;
     return removeAnnotation(Inst, *Index);
   }
 
-  /// Remove meta-data, but don't destroy it.
-  void stripAnnotations(MCInst &Inst, bool KeepTC = false);
+  /// Remove meta-data from the instruction, but don't destroy it.
+  void stripAnnotations(MCInst &Inst, bool KeepTC = false) const;
 
   virtual InstructionListType
   createInstrumentedIndirectCall(MCInst &&CallInst, MCSymbol *HandlerFuncAddr,

bolt/lib/Core/BinaryFunction.cpp

@@ -1999,7 +1999,7 @@ bool BinaryFunction::buildCFG(MCPlusBuilder::AllocatorIdTy AllocatorId) {
       }
     }
     if (LastNonNop && !MIB->getOffset(*LastNonNop))
-      MIB->setOffset(*LastNonNop, static_cast<uint32_t>(Offset), AllocatorId);
+      MIB->setOffset(*LastNonNop, static_cast<uint32_t>(Offset));
   };
 
   for (auto I = Instructions.begin(), E = Instructions.end(); I != E; ++I) {
@@ -2022,7 +2022,7 @@ bool BinaryFunction::buildCFG(MCPlusBuilder::AllocatorIdTy AllocatorId) {
     if (MIB->isNoop(Instr) && !MIB->getOffset(Instr)) {
       // If "Offset" annotation is not present, set it and mark the nop for
       // deletion.
-      MIB->setOffset(Instr, static_cast<uint32_t>(Offset), AllocatorId);
+      MIB->setOffset(Instr, static_cast<uint32_t>(Offset));
       // Annotate ordinary nops, so we can safely delete them if required.
       MIB->addAnnotation(Instr, "NOP", static_cast<uint32_t>(1), AllocatorId);
     }
@@ -2303,6 +2303,13 @@ void BinaryFunction::removeConditionalTailCalls() {
     assert(CTCTargetLabel && "symbol expected for conditional tail call");
     MCInst TailCallInstr;
     BC.MIB->createTailCall(TailCallInstr, CTCTargetLabel, BC.Ctx.get());
+
+    // Move offset from CTCInstr to TailCallInstr.
+    if (const std::optional<uint32_t> Offset = BC.MIB->getOffset(*CTCInstr)) {
+      BC.MIB->setOffset(TailCallInstr, *Offset);
+      BC.MIB->clearOffset(*CTCInstr);
+    }
+
     // Link new BBs to the original input offset of the BB where the CTC
     // is, so we can map samples recorded in new BBs back to the original BB
     // seem in the input binary (if using BAT)
@@ -2331,12 +2338,6 @@ void BinaryFunction::removeConditionalTailCalls() {
 
     // This branch is no longer a conditional tail call.
     BC.MIB->unsetConditionalTailCall(*CTCInstr);
-
-    // Move offset from CTCInstr to TailCallInstr.
-    if (std::optional<uint32_t> Offset = BC.MIB->getOffset(*CTCInstr)) {
-      BC.MIB->setOffset(TailCallInstr, *Offset);
-      BC.MIB->clearOffset(*CTCInstr);
-    }
   }
 
   insertBasicBlocks(std::prev(end()), std::move(NewBlocks),
@@ -3373,7 +3374,7 @@ void BinaryFunction::propagateGnuArgsSizeInfo(
         }
       } else if (BC.MIB->isInvoke(Instr)) {
         // Add the value of GNU_args_size as an extra operand to invokes.
-        BC.MIB->addGnuArgsSize(Instr, CurrentGnuArgsSize, AllocId);
+        BC.MIB->addGnuArgsSize(Instr, CurrentGnuArgsSize);
       }
       ++II;
     }