[MachineInstrBuilder] Introduce MIMetadata to simplify metadata propagation

[melver]

Summary

In many places DebugLoc and PCSections metadata are just copied along to
propagate them through MachineInstrs. Simplify doing so by bundling them
up in a MIMetadata class that replaces the DebugLoc argument to most
BuildMI() variants.

The DebugLoc-only constructors allow implicit construction, so that
existing usage of BuildMI(.., DL, ..) works as before, and the rest of
the codebase using BuildMI() does not require changes.

NFC.

Depends on D130882

[melver]

Rebase.

[melver]

Rebase.

[vitalybuka]

Would you like to extend llvm/unittests/CodeGen/MachineInstrTest.cpp ?

[melver]

Add simple BuildMI() unit test.

PTAL.

[vitalybuka]

LGTM, but please blocking reviews if you are looking for their feedback in particular

llvm/unittests/CodeGen/MachineInstrTest.cpp

463

I don't insist, but you want to avoid such wrapping of EXPECTs into functions or lambdas.
When test fails it will not provide location of the failed line with CheckMI

If you want to reuse pair of checks,
EXPECT_EQ(MIB->getDebugLoc(), DL);
EXPECT_EQ(MIB->getPCSections(), PCS);

you can use EXPECT_THAT with custom matches
EXPECT_THAT(BuildMI(*MBB, MBB->end(), MIMD, MCID)(), MyMatches);

another alternative:

 auto BuildDiPc = [&](...) -> pair<> {
    MachineInstrBuilder MIB = BuildMI(...)();
  return {MIB->getDebugLoc(), MIB->getPCSections()}
  };

EXPECT_EQ({DL, PC}. BuildDiPc(*MBB, MBB->end(), MIMD, MCID));

[melver]

Use EXPECT_THAT() with custom matcher for MIMetadata in test.

[melver]

llvm/unittests/CodeGen/MachineInstrTest.cpp

463

Thanks, updated to use EXPECT_THAT.

llvm/include/llvm/CodeGen/MachineInstrBuilder.h

switch (Disp.getType()) {
         return addBlockAddress(Disp.getBlockAddress(), Disp.getOffset() + off,
                                TargetFlags);
       case MachineOperand::MO_JumpTableIndex:
         assert(off == 0 && "cannot create offset into jump tables");
         return addJumpTableIndex(Disp.getIndex(), TargetFlags);
     }
   }
 
+  const MachineInstrBuilder &setPCSections(MDNode *MD) const {
+    if (MD)
+      MI->setPCSections(*MF, MD);
+    return *this;
+  }
+
   /// Copy all the implicit operands from OtherMI onto this one.
   const MachineInstrBuilder &
   copyImplicitOps(const MachineInstr &OtherMI) const {
     MI->copyImplicitOps(*MF, OtherMI);
     return *this;
   }
 
   bool constrainAllUses(const TargetInstrInfo &TII,
                         const TargetRegisterInfo &TRI,
                         const RegisterBankInfo &RBI) const {
     return constrainSelectedInstRegOperands(*MI, TII, TRI, RBI);
   }
 };
 
+/// Set of metadata that should be preserved when using BuildMI(). This provides
+/// a more convenient way of preserving DebugLoc and PCSections.
+class MIMetadata {
+public:
+  MIMetadata() = default;
+  MIMetadata(DebugLoc DL, MDNode *PCSections = nullptr)
+      : DL(std::move(DL)), PCSections(PCSections) {}
+  MIMetadata(const DILocation *DI, MDNode *PCSections = nullptr)
+      : DL(DI), PCSections(PCSections) {}
+  explicit MIMetadata(const Instruction &From)
+      : DL(From.getDebugLoc()),
+        PCSections(From.getMetadata(LLVMContext::MD_pcsections)) {}
+  explicit MIMetadata(const MachineInstr &From)
+      : DL(From.getDebugLoc()), PCSections(From.getPCSections()) {}
+
+  const DebugLoc &getDL() const { return DL; }
+  MDNode *getPCSections() const { return PCSections; }
+
+private:
+  DebugLoc DL;
+  MDNode *PCSections = nullptr;
+};
+
 /// Builder interface. Specify how to create the initial instruction itself.
-inline MachineInstrBuilder BuildMI(MachineFunction &MF, const DebugLoc &DL,
+inline MachineInstrBuilder BuildMI(MachineFunction &MF, const MIMetadata &MIMD,
                                    const MCInstrDesc &MCID) {
-  return MachineInstrBuilder(MF, MF.CreateMachineInstr(MCID, DL));
+  return MachineInstrBuilder(MF, MF.CreateMachineInstr(MCID, MIMD.getDL()))
+           .setPCSections(MIMD.getPCSections());
 }
 
 /// This version of the builder sets up the first operand as a
 /// destination virtual register.
-inline MachineInstrBuilder BuildMI(MachineFunction &MF, const DebugLoc &DL,
+inline MachineInstrBuilder BuildMI(MachineFunction &MF, const MIMetadata &MIMD,
                                    const MCInstrDesc &MCID, Register DestReg) {
-  return MachineInstrBuilder(MF, MF.CreateMachineInstr(MCID, DL))
+  return MachineInstrBuilder(MF, MF.CreateMachineInstr(MCID, MIMD.getDL()))
+           .setPCSections(MIMD.getPCSections())
            .addReg(DestReg, RegState::Define);
 }
 
 /// This version of the builder inserts the newly-built instruction before
 /// the given position in the given MachineBasicBlock, and sets up the first
 /// operand as a destination virtual register.
 inline MachineInstrBuilder BuildMI(MachineBasicBlock &BB,
                                    MachineBasicBlock::iterator I,
-                                   const DebugLoc &DL, const MCInstrDesc &MCID,
-                                   Register DestReg) {
+                                   const MIMetadata &MIMD,
+                                   const MCInstrDesc &MCID, Register DestReg) {
   MachineFunction &MF = *BB.getParent();
-  MachineInstr *MI = MF.CreateMachineInstr(MCID, DL);
+  MachineInstr *MI = MF.CreateMachineInstr(MCID, MIMD.getDL());
   BB.insert(I, MI);
-  return MachineInstrBuilder(MF, MI).addReg(DestReg, RegState::Define);
+  return MachineInstrBuilder(MF, MI)
+           .setPCSections(MIMD.getPCSections())
+           .addReg(DestReg, RegState::Define);
 }
 
 /// This version of the builder inserts the newly-built instruction before
 /// the given position in the given MachineBasicBlock, and sets up the first
 /// operand as a destination virtual register.
 ///
 /// If \c I is inside a bundle, then the newly inserted \a MachineInstr is
 /// added to the same bundle.
 inline MachineInstrBuilder BuildMI(MachineBasicBlock &BB,
                                    MachineBasicBlock::instr_iterator I,
-                                   const DebugLoc &DL, const MCInstrDesc &MCID,
-                                   Register DestReg) {
+                                   const MIMetadata &MIMD,
+                                   const MCInstrDesc &MCID, Register DestReg) {
   MachineFunction &MF = *BB.getParent();
-  MachineInstr *MI = MF.CreateMachineInstr(MCID, DL);
+  MachineInstr *MI = MF.CreateMachineInstr(MCID, MIMD.getDL());
   BB.insert(I, MI);
-  return MachineInstrBuilder(MF, MI).addReg(DestReg, RegState::Define);
+  return MachineInstrBuilder(MF, MI)
+           .setPCSections(MIMD.getPCSections())
+           .addReg(DestReg, RegState::Define);
 }
 
 inline MachineInstrBuilder BuildMI(MachineBasicBlock &BB, MachineInstr &I,
-                                   const DebugLoc &DL, const MCInstrDesc &MCID,
-                                   Register DestReg) {
+                                   const MIMetadata &MIMD,
+                                   const MCInstrDesc &MCID, Register DestReg) {
   // Calling the overload for instr_iterator is always correct.  However, the
   // definition is not available in headers, so inline the check.
   if (I.isInsideBundle())
-    return BuildMI(BB, MachineBasicBlock::instr_iterator(I), DL, MCID, DestReg);
-  return BuildMI(BB, MachineBasicBlock::iterator(I), DL, MCID, DestReg);
+    return BuildMI(BB, MachineBasicBlock::instr_iterator(I), MIMD, MCID,
+                   DestReg);
+  return BuildMI(BB, MachineBasicBlock::iterator(I), MIMD, MCID, DestReg);
 }
 
 inline MachineInstrBuilder BuildMI(MachineBasicBlock &BB, MachineInstr *I,
-                                   const DebugLoc &DL, const MCInstrDesc &MCID,
-                                   Register DestReg) {
-  return BuildMI(BB, *I, DL, MCID, DestReg);
+                                   const MIMetadata &MIMD,
+                                   const MCInstrDesc &MCID, Register DestReg) {
+  return BuildMI(BB, *I, MIMD, MCID, DestReg);
 }
 
 /// This version of the builder inserts the newly-built instruction before the
 /// given position in the given MachineBasicBlock, and does NOT take a
 /// destination register.
 inline MachineInstrBuilder BuildMI(MachineBasicBlock &BB,
                                    MachineBasicBlock::iterator I,
-                                   const DebugLoc &DL,
+                                   const MIMetadata &MIMD,
                                    const MCInstrDesc &MCID) {
   MachineFunction &MF = *BB.getParent();
-  MachineInstr *MI = MF.CreateMachineInstr(MCID, DL);
+  MachineInstr *MI = MF.CreateMachineInstr(MCID, MIMD.getDL());
   BB.insert(I, MI);
-  return MachineInstrBuilder(MF, MI);
+  return MachineInstrBuilder(MF, MI).setPCSections(MIMD.getPCSections());
 }
 
 inline MachineInstrBuilder BuildMI(MachineBasicBlock &BB,
                                    MachineBasicBlock::instr_iterator I,
-                                   const DebugLoc &DL,
+                                   const MIMetadata &MIMD,
                                    const MCInstrDesc &MCID) {
   MachineFunction &MF = *BB.getParent();
-  MachineInstr *MI = MF.CreateMachineInstr(MCID, DL);
+  MachineInstr *MI = MF.CreateMachineInstr(MCID, MIMD.getDL());
   BB.insert(I, MI);
-  return MachineInstrBuilder(MF, MI);
+  return MachineInstrBuilder(MF, MI).setPCSections(MIMD.getPCSections());
 }
 
 inline MachineInstrBuilder BuildMI(MachineBasicBlock &BB, MachineInstr &I,
-                                   const DebugLoc &DL,
+                                   const MIMetadata &MIMD,
                                    const MCInstrDesc &MCID) {
   // Calling the overload for instr_iterator is always correct.  However, the
   // definition is not available in headers, so inline the check.
   if (I.isInsideBundle())
-    return BuildMI(BB, MachineBasicBlock::instr_iterator(I), DL, MCID);
-  return BuildMI(BB, MachineBasicBlock::iterator(I), DL, MCID);
+    return BuildMI(BB, MachineBasicBlock::instr_iterator(I), MIMD, MCID);
+  return BuildMI(BB, MachineBasicBlock::iterator(I), MIMD, MCID);
 }
 
 inline MachineInstrBuilder BuildMI(MachineBasicBlock &BB, MachineInstr *I,
-                                   const DebugLoc &DL,
+                                   const MIMetadata &MIMD,
                                    const MCInstrDesc &MCID) {
-  return BuildMI(BB, *I, DL, MCID);
+  return BuildMI(BB, *I, MIMD, MCID);
 }
 
 /// This version of the builder inserts the newly-built instruction at the end
 /// of the given MachineBasicBlock, and does NOT take a destination register.
-inline MachineInstrBuilder BuildMI(MachineBasicBlock *BB, const DebugLoc &DL,
+inline MachineInstrBuilder BuildMI(MachineBasicBlock *BB,
+                                   const MIMetadata &MIMD,
                                    const MCInstrDesc &MCID) {
-  return BuildMI(*BB, BB->end(), DL, MCID);
+  return BuildMI(*BB, BB->end(), MIMD, MCID);
 }
 
 /// This version of the builder inserts the newly-built instruction at the
 /// end of the given MachineBasicBlock, and sets up the first operand as a
 /// destination virtual register.
-inline MachineInstrBuilder BuildMI(MachineBasicBlock *BB, const DebugLoc &DL,
+inline MachineInstrBuilder BuildMI(MachineBasicBlock *BB,
+                                   const MIMetadata &MIMD,
                                    const MCInstrDesc &MCID, Register DestReg) {
-  return BuildMI(*BB, BB->end(), DL, MCID, DestReg);
+  return BuildMI(*BB, BB->end(), MIMD, MCID, DestReg);
 }
 
 /// This version of the builder builds a DBG_VALUE intrinsic
 /// for either a value in a register or a register-indirect
 /// address.  The convention is that a DBG_VALUE is indirect iff the
 /// second operand is an immediate.
 MachineInstrBuilder BuildMI(MachineFunction &MF, const DebugLoc &DL,
                             const MCInstrDesc &MCID, bool IsIndirect,

llvm/unittests/CodeGen/MachineInstrTest.cpp

 //===- MachineInstrTest.cpp -----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineMemOperand.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/CodeGen/TargetFrameLowering.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/CodeGen/TargetLowering.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/ModuleSlotTracker.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetOptions.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 namespace {
 // Include helper functions to ease the manipulation of MachineFunctions.
 #include "MFCommon.inc"
 
for (const unsigned short Opcode :
     MI->addOperand(*MF, MachineOperand::CreateImm(0));
     MI->getOperand(1).ChangeToRegister(0, false);
 
     ASSERT_TRUE(MI->getOperand(0).isDebug());
     ASSERT_TRUE(MI->getOperand(1).isDebug());
   }
 }
 
+MATCHER_P(HasMIMetadata, MIMD, "") {
+  return arg->getDebugLoc() == MIMD.getDL() &&
+         arg->getPCSections() == MIMD.getPCSections();
+}
+
+TEST(MachineInstrBuilder, BuildMI) {
+  LLVMContext Ctx;
+  MDNode *PCS = MDNode::getDistinct(Ctx, None);
+  MDNode *DI = MDNode::getDistinct(Ctx, None);
+  DebugLoc DL(DI);
+  MIMetadata MIMD(DL, PCS);
+  EXPECT_EQ(MIMD.getDL(), DL);
+  EXPECT_EQ(MIMD.getPCSections(), PCS);
+  // Check common BuildMI() overloads propagate MIMetadata.
+  Module Mod("Module", Ctx);
+  auto MF = createMachineFunction(Ctx, Mod);
+  auto MBB = MF->CreateMachineBasicBlock();
+  MCInstrDesc MCID = {0, 0, 0, 0, 0, 0, 0, nullptr, nullptr, nullptr};
+  EXPECT_THAT(BuildMI(*MF, MIMD, MCID), HasMIMetadata(MIMD));
+  EXPECT_THAT(BuildMI(*MF, MIMD, MCID), HasMIMetadata(MIMD));
+  EXPECT_THAT(BuildMI(*MBB, MBB->end(), MIMD, MCID), HasMIMetadata(MIMD));
+  EXPECT_THAT(BuildMI(*MBB, MBB->end(), MIMD, MCID), HasMIMetadata(MIMD));
+  EXPECT_THAT(BuildMI(*MBB, MBB->instr_end(), MIMD, MCID), HasMIMetadata(MIMD));
+  EXPECT_THAT(BuildMI(*MBB, *MBB->begin(), MIMD, MCID), HasMIMetadata(MIMD));
+  EXPECT_THAT(BuildMI(*MBB, &*MBB->begin(), MIMD, MCID), HasMIMetadata(MIMD));
+  EXPECT_THAT(BuildMI(MBB, MIMD, MCID), HasMIMetadata(MIMD));
+}
+
 static_assert(std::is_trivially_copyable<MCOperand>::value,
               "trivially copyable");
 
 } // end namespace