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

1159 ↗

(On Diff #80982)

How about "Dumper that prevents ..."

1161 ↗

(On Diff #80982)

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

llvm/lib/CodeGen/MachineInstr.cpp

1705 ↗

(On Diff #80982)

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

1718 ↗

(On Diff #80982)

if (!TII) TII = ...

1746 ↗

(On Diff #80982)

The entire statement can be removed

1999 ↗

(On Diff #80982)

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 ↗

(On Diff #81111)

Target... *const vs * const?

llvm/lib/CodeGen/MachineInstr.cpp

1705 ↗

(On Diff #81111)

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 ↗

(On Diff #81268)

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

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/trunk/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 dump() const;
+  void print(raw_ostream &OS, bool SkipOpers = false,
+             const TargetInstrInfo *TII = nullptr) const;
+  void print(raw_ostream &OS, ModuleSlotTracker &MST, bool SkipOpers = false,
+             const TargetInstrInfo *TII = nullptr) const;
+  void dump(const TargetInstrInfo *TII = nullptr) 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/trunk/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/trunk/lib/CodeGen/MachineInstr.cpp

void MachineInstr::copyImplicitOps(MachineFunction &MF,
   for (unsigned i = MI.getDesc().getNumOperands(), e = MI.getNumOperands();
        i != e; ++i) {
     const MachineOperand &MO = MI.getOperand(i);
     if ((MO.isReg() && MO.isImplicit()) || MO.isRegMask())
       addOperand(MF, MO);
   }
 }
 
-LLVM_DUMP_METHOD void MachineInstr::dump() const {
+LLVM_DUMP_METHOD void MachineInstr::dump(const TargetInstrInfo *TII) const {
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
-  dbgs() << "  " << *this;
+  dbgs() << "  ";
+  print(dbgs(), false /* SkipOpers */, TII);
 #endif
 }
 
-void MachineInstr::print(raw_ostream &OS, bool SkipOpers) const {
+void MachineInstr::print(raw_ostream &OS, bool SkipOpers,
+                         const TargetInstrInfo *TII) const {
   const Module *M = nullptr;
   if (const MachineBasicBlock *MBB = getParent())
     if (const MachineFunction *MF = MBB->getParent())
       M = MF->getFunction()->getParent();
 
   ModuleSlotTracker MST(M);
-  print(OS, MST, SkipOpers);
+  print(OS, MST, SkipOpers, TII);
 }
 
 void MachineInstr::print(raw_ostream &OS, ModuleSlotTracker &MST,
-                         bool SkipOpers) const {
+                         bool SkipOpers, const TargetInstrInfo *TII) 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();
+      if (!TII)
         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.

llvm/trunk/test/CodeGen/AArch64/machine-combiner-madd.ll

 ; Test all AArch64 subarches with scheduling models.
 ; RUN: llc -mtriple=aarch64-linux-gnu -mcpu=cortex-a57 < %s | FileCheck %s
 ; RUN: llc -mtriple=aarch64-linux-gnu -mcpu=cortex-a72 < %s | FileCheck %s
 ; RUN: llc -mtriple=aarch64-linux-gnu -mcpu=cortex-a73 < %s | FileCheck %s
 ; RUN: llc -mtriple=aarch64-linux-gnu -mcpu=cyclone    < %s | FileCheck %s
 ; RUN: llc -mtriple=aarch64-linux-gnu -mcpu=exynos-m1  < %s | FileCheck %s
 ; RUN: llc -mtriple=aarch64-linux-gnu -mcpu=exynos-m2  < %s | FileCheck %s
 ; RUN: llc -mtriple=aarch64-linux-gnu -mcpu=kryo       < %s | FileCheck %s
 ; RUN: llc -mtriple=aarch64-linux-gnu -mcpu=vulcan     < %s | FileCheck %s
+; RUN: llc -mtriple=aarch64-linux-gnu -mcpu=cortex-a57 -debug-only=machine-combiner < %s 2>&1 > /dev/null | FileCheck %s --check-prefix COMBINE-DUMP
+
+; The machine-combiner debugging dumps should print the new instruction.
+; COMBINE-DUMP: NEW INSTR{{.*}}MADDXrrr
+; COMBINE-DUMP-NOT: NEW INSTR{{.*}}UNKNOWN
 
 ; Make sure that inst-combine fuses the multiply add in the addressing mode of
 ; the load.
 
 ; CHECK-LABEL: fun:
 ; CHECK-NOT: mul
 ; CHECK:     madd
 ; CHECK-NOT: mul