debug output incorrect ["UNKNOWN"] when a MachineInstr is generated during the MachineCombiner pass and the relevant debug output is enabled

[sebpop]

Summary

Some of the LLVM debug output is incorrect ["UNKNOWN"] when a MachineInstr is generated during the MachineCombiner pass and the relevant debug output is enabled.

The fix is generic, but the test case [based on machine-reduced Eigen3 C++ code] is AArch64-specific.

Debug output before the fix:

NEW INSTR %vreg15<def> = UNKNOWN %vreg1, %vreg13<kill>, %vreg11<kill>

Debug output after the fix:

NEW INSTR %vreg15<def> = MADDXrrr %vreg1, %vreg13<kill>, %vreg11<kill>

I [Abe] searched hard for a less-kludgy way to accomplish this, but did not find one.

[Gerolf]

llvm/include/llvm/CodeGen/MachineInstr.h

1157

How about "Dumper that prevents ..."

1159

Why not overlay dump(...)? At least use naming convention eg. dumpWithGiven...

llvm/lib/CodeGen/MachineInstr.cpp

1705

This can be simpler: use default initializer third parameter TTI = nullptr

1718

if (!TII) TII = ...

1746

The entire statement can be removed

1999

use TTI rather then TTI_in

[Abe]

Integrated some good suggestions from Gerolf, thereby making the fix less kludgy.

[echristo]

I'd suggest formatting with clang-format as well.

[sebpop]

llvm/test/CodeGen/AArch64/correct_debug_output_when_a_new_MachineInstr_is_generated_in_the_machine_combiner.ll

1 ↗

(On Diff #80989)

Please replace this testcase with the one that I reduced for the other instr-combiner patch:
llvm/test/CodeGen/AArch64/machine-combiner-madd.ll

Also, please use reasonably shorter file names.

5 ↗

(On Diff #80989)

Restrict the flags of the run command to a minimum. Please see the RUN command in the above mentioned ll file.

[Abe]

Implemented some recommendations/suggestions from Sebastian.

[Abe]

Incorporated Sebastian`s further reduction of the new test case.

[Abe]

Now using "= nullptr" instead of "= 0" in one spot.

[Gerolf]

llvm/include/llvm/CodeGen/MachineInstr.h

1160

Target... *const vs * const?

llvm/lib/CodeGen/MachineInstr.cpp

1705

Shouldn't it be ...Target... *const vs * const?

When TTI_in is not nullptr *and* TII actually does get defined in the function, how would TTI_in be different from TII?

Also TTI_in is declared differently from TII? Why?

[Abe]

Minor changes in source code formatting, editing according to "clang-format" output [ran with "-style=LLVM"].

[Abe]

Changed the interface for the new "MachineInstr::print(...)" overload as Gerolf requested.

[Abe]

Trivial edit; NFC WRT prev. diff in same patch.

[Gerolf]

llvm/lib/CodeGen/MachineInstr.cpp

1724

I think this confirms that you want to initialize the TII with the nullptr.
Then remove const .. *TII in line 1710.
Finally you can remove if (TTI_).

[Abe]

fixed a bug Gerolf caught

[sebpop]

Taking over this patch: I will update the patch addressing all the comments from Gerolf.

[sebpop]

Fixed the problems that Gerolf has asked to be addressed, and reduced the number of changes.

[Gerolf]

Thanks! LGTM.

[MatzeB]

With this change in place the a typical lldb operation fails now:

lldb > p I->dump()
error: too few arguments to function call, expected 1, have 0

It looks like lldb is not able to put in the default value. However as this can be really annoying for developers and seems unnecessary, can we revert the changes to the dump() function and only keep the extra parameter on the print() function? (MachineCombiner.cpp really should be chaned to the simpler dbgs() << *InstrPtr instead of using the debug helper dump())

[sebpop]

llvm/trunk/include/llvm/CodeGen/MachineInstr.h

1156 ↗

(On Diff #82186)

If you don't pass TII to dump() it should just get nullptr as a default value.

[sebpop]

In D27645#652158, @MatzeB wrote:

With this change in place the a typical lldb operation fails now:

lldb > p I->dump()
error: too few arguments to function call, expected 1, have 0

I suspect this is an error in lldb. Please try with gdb to confirm.

[MatzeB]

In D27645#652223, @sebpop wrote:

In D27645#652158, @MatzeB wrote:

With this change in place the a typical lldb operation fails now:

lldb > p I->dump()
error: too few arguments to function call, expected 1, have 0

I suspect this is an error in lldb. Please try with gdb to confirm.

Getting a working gdb version on macOS is hard (last time I tried, I could not get code signing to work in a way that allowed gdb to actually debug processes). So I cannot really test.
Even though this is clearly a deficiency in lldb, using MI.dump() in a debugger is such a common operation, that I'd rather have the dump() function changed.

llvm/include/llvm/CodeGen/MachineInstr.h

   /// Copy implicit register operands from specified
   /// instruction to this instruction.
   void copyImplicitOps(MachineFunction &MF, const MachineInstr &MI);
 
   //
   // Debugging support
   //
   void print(raw_ostream &OS, bool SkipOpers = false) const;
-  void print(raw_ostream &OS, ModuleSlotTracker &MST,
-             bool SkipOpers = false) const;
+  void print(raw_ostream &OS, ModuleSlotTracker &MST, bool SkipOpers = false,
+             const TargetInstrInfo *const = nullptr) const;
   void dump() const;
 
+  // A dumper that prevents UNKNOWN when outputting
+  // an MI that is a result of combining MIs.
+  // Possibly useful for other situations when the TII* cannot be computed.
+  void dump(const TargetInstrInfo *const) const;
+
   //===--------------------------------------------------------------------===//
   // Accessors used to build up machine instructions.
 
   /// Add the specified operand to the instruction.  If it is an implicit
   /// operand, it is added to the end of the operand list.  If it is an
   /// explicit operand it is added at the end of the explicit operand list
   /// (before the first implicit operand).
   ///

llvm/lib/CodeGen/MachineCombiner.cpp

MachineCombiner::getDepth(SmallVectorImpl<MachineInstr *> &InsInstrs,
   assert(TSchedModel.hasInstrSchedModelOrItineraries() &&
          "Missing machine model\n");
 
   // For each instruction in the new sequence compute the depth based on the
   // operands. Use the trace information when possible. For new operands which
   // are tracked in the InstrIdxForVirtReg map depth is looked up in InstrDepth
   for (auto *InstrPtr : InsInstrs) { // for each Use
     unsigned IDepth = 0;
-    DEBUG(dbgs() << "NEW INSTR "; InstrPtr->dump(); dbgs() << "\n";);
+    DEBUG(dbgs() << "NEW INSTR "; InstrPtr->dump(TII); dbgs() << "\n";);
     for (const MachineOperand &MO : InstrPtr->operands()) {
       // Check for virtual register operand.
       if (!(MO.isReg() && TargetRegisterInfo::isVirtualRegister(MO.getReg())))
         continue;
       if (!MO.isUse())
         continue;
       unsigned DepthOp = 0;
       unsigned LatencyOp = 0;

llvm/lib/CodeGen/MachineInstr.cpp

if (const MachineBasicBlock *MBB = getParent())
     if (const MachineFunction *MF = MBB->getParent())
       M = MF->getFunction()->getParent();
 
   ModuleSlotTracker MST(M);
   print(OS, MST, SkipOpers);
 }
 
 void MachineInstr::print(raw_ostream &OS, ModuleSlotTracker &MST,
-                         bool SkipOpers) const {
+                         bool SkipOpers,
+                         const TargetInstrInfo *const TII_in) const {
   // We can be a bit tidier if we know the MachineFunction.
   const MachineFunction *MF = nullptr;
   const TargetRegisterInfo *TRI = nullptr;
   const MachineRegisterInfo *MRI = nullptr;
   const TargetInstrInfo *TII = nullptr;
   const TargetIntrinsicInfo *IntrinsicInfo = nullptr;
 
   if (const MachineBasicBlock *MBB = getParent()) {
     MF = MBB->getParent();
     if (MF) {
       MRI = &MF->getRegInfo();
       TRI = MF->getSubtarget().getRegisterInfo();
       TII = MF->getSubtarget().getInstrInfo();
       IntrinsicInfo = MF->getTarget().getIntrinsicInfo();
     }
   }
 
   // Save a list of virtual registers.
   SmallVector<unsigned, 8> VirtRegs;
 
   // Print explicitly defined operands on the left of an assignment syntax.
   unsigned StartOp = 0, e = getNumOperands();
   for (; StartOp < e && getOperand(StartOp).isReg() &&
          getOperand(StartOp).isDef() &&
          !getOperand(StartOp).isImplicit();
        ++StartOp) {
     if (StartOp != 0) OS << ", ";
     getOperand(StartOp).print(OS, MST, TRI, IntrinsicInfo);
     unsigned Reg = getOperand(StartOp).getReg();
     if (TargetRegisterInfo::isVirtualRegister(Reg)) {
       VirtRegs.push_back(Reg);
       LLT Ty = MRI ? MRI->getType(Reg) : LLT{};
       if (Ty.isValid())
         OS << '(' << Ty << ')';
     }
   }
 
   if (StartOp != 0)
     OS << " = ";
 
+  if (!TII)
+    TII = TII_in;
+
   // Print the opcode name.
   if (TII)
     OS << TII->getName(getOpcode());
   else
     OS << "UNKNOWN";
 
   if (SkipOpers)
     return;
if (!HaveSemi)
       OS << ";";
     OS << " dbg:";
     debugLoc.print(OS);
   }
 
   OS << '\n';
 }
 
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+void MachineInstr::dump(const TargetInstrInfo *const TII_in) const {
+  dbgs() << "  ";
+  const Module *M = nullptr;
+  if (const MachineBasicBlock *MBB = getParent())
+    if (const MachineFunction *MF = MBB->getParent())
+      M = MF->getFunction()->getParent();
+
+  ModuleSlotTracker MST(M);
+  print(dbgs(), MST, false /* SkipOpers */, TII_in);
+}
+#endif
+
 bool MachineInstr::addRegisterKilled(unsigned IncomingReg,
                                      const TargetRegisterInfo *RegInfo,
                                      bool AddIfNotFound) {
   bool isPhysReg = TargetRegisterInfo::isPhysicalRegister(IncomingReg);
   bool hasAliases = isPhysReg &&
     MCRegAliasIterator(IncomingReg, RegInfo, false).isValid();
   bool Found = false;
   SmallVector<unsigned,4> DeadOps;

llvm/test/CodeGen/AArch64/debug_output_upon_new-MachineInstr-from-combiner.ll

+; Check for correct debug output when
+; a new MachineInstr is generated in the machine combiner.
+
+; Converted from a machine-reduced C++ test case.
+
+; The fix is actually ISA-agnostic, but this test is AArch64-specific.
+
+; RUN: llc -mcpu=cortex-a57 -debug-only=machine-combiner -o - %s |& FileCheck %s
+
+; CHECK:     NEW INSTR{{.*}}MADDXrrr
+; CHECK-NOT: NEW INSTR{{.*}}UNKNOWN
+
+target triple = "aarch64-linux-gnu"
+
+%class.D = type { %class.basic_string.base, [4 x i8] }
+%class.basic_string.base = type <{ i64, i64, i32 }>
+@a = global %class.D* zeroinitializer, align 8
+declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture writeonly, i8* nocapture readonly, i64, i32, i1)
+define internal void @fun() section ".text.startup" {
+entry:
+  %tmp.i.i = alloca %class.D, align 8
+  %y = bitcast %class.D* %tmp.i.i to i8*
+  br label %loop
+loop:
+  %conv11.i.i = phi i64 [ 0, %entry ], [ %inc.i.i, %loop ]
+  %i = phi i64 [ undef, %entry ], [ %inc.i.i, %loop ]
+  %x = load %class.D*, %class.D** getelementptr inbounds (%class.D*, %class.D** @a, i64 0), align 8
+  %arrayidx.i.i.i = getelementptr inbounds %class.D, %class.D* %x, i64 %conv11.i.i
+  %d = bitcast %class.D* %arrayidx.i.i.i to i8*
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* nonnull %y, i8* %d, i64 24, i32 8, i1 false)
+  %inc.i.i = add i64 %i, 1
+  %cmp.i.i = icmp slt i64 %inc.i.i, 0
+  br i1 %cmp.i.i, label %loop, label %exit
+exit:
+  ret void
+}