[PATCH] D41278: [MachineCombiner] Improve debug output (NFC)

Wed Jan 24 02:56:47 PST 2018

fhahn added a comment.

Debug output with this patch for `test/CodeGen/AArch64/machine-combiner.mir`. I think it's an overall improvement , I just have a few more comments. I think it also would be worth explicitly stating that we applied a substitution.

  Machine InstCombiner: inc_update_iterator_test
  Combining MBB
  Combining MBB
   Possible instr to replace
  	 sched: [1:0.50]: %6:gpr32 = ADDWrr %3, killed %5Computing MinInstr trace through %bb.1
    pred for %bb.0: null
    pred for %bb.1: %bb.0
    succ for %bb.1: null

  Depths for %bb.0:
        0 Instructions
  0	%3:gpr32 = COPY %w2
  0	%2:gpr32 = COPY %w1
  0	%1:gpr32 = COPY %w0
  0	%0:fpr64 = COPY %d0
  0	%4:gpr32 = SUBSWrr %1:gpr32, %2:gpr32, implicit-def %nzcv
  1	Bcc 13, %bb.2, implicit %nzcv
  0	B %bb.1

  Depths for %bb.1:
        3 Instructions
       2c @ A57UnitB (2 ops x6)
       1c @ A57UnitI (1 ops x3)
  0	%5:gpr32 = MADDWrrr %1:gpr32, %2:gpr32, %wzr
  3	%6:gpr32 = ADDWrr %3:gpr32, killed %5:gpr32
  4	%7:fpr64 = SCVTFUWDri killed %6:gpr32
  14	%8:fpr64 = FMULDrr %7:fpr64, %7:fpr64
  19	%9:fpr64 = FADDDrr %0:fpr64, killed %8:fpr64
  24	%d0 = COPY %9:fpr64
  24	RET_ReallyLR implicit %d0
  Heights for %bb.1:
        6 Instructions
       1c @ A57UnitB (1 ops x6)
       1c @ A57UnitI (1 ops x3)
       1c @ A57UnitL (1 ops x6)
       1c @ A57UnitM (1 ops x6)
       2c @ A57UnitV (3 ops x3)
  24	0	RET_ReallyLR implicit %d0
  24	0	%d0 = COPY %9:fpr64
  24	5	%9:fpr64 = FADDDrr %0:fpr64, killed %8:fpr64
  24	10	%8:fpr64 = FMULDrr %7:fpr64, %7:fpr64
  24	20	%7:fpr64 = SCVTFUWDri killed %6:gpr32
  24	21	%6:gpr32 = ADDWrr %3:gpr32, killed %5:gpr32
  24	24	%5:gpr32 = MADDWrrr %1:gpr32, %2:gpr32, %wzr
  %bb.1 Live-ins: %0 at 5 %3 at 21 %1 at 24 %2 at 24 WZR at 24
  Critical path: 24
    Dependence data for %6:gpr32 = ADDWrr %3:gpr32, killed %5:gpr32
  	NewRootDepth: 1	RootDepth: 3
  	NewRootLatency: 3	RootLatency: 4
  	RootSlack: 0 SlackIsAccurate=1
  	NewRootDepth + NewRootLatency = 4
  	RootDepth + RootLatency + RootSlack = 7
  	  It IMPROVES PathLen because
  		NewCycleCount = 4, OldCycleCount = 7
  		Resource length before replacement: 5 and after: 4
  		  As result it IMPROVES/PRESERVES Resource Length
  Invalidate %bb.1 MinInstr height.
  Invalidate %bb.1 MinInstr depth.
   Possible instr to replace
  	 sched: [5:0.50]: %9:fpr64 = FADDDrr %0, killed %8Computing MinInstr trace through %bb.1
    pred for %bb.1: %bb.0
    succ for %bb.1: null

  Depths for %bb.1:
        3 Instructions
       2c @ A57UnitB (2 ops x6)
       1c @ A57UnitI (1 ops x3)
  0	%6:gpr32 = MADDWrrr %1:gpr32, %2:gpr32, %3:gpr32
  3	%7:fpr64 = SCVTFUWDri killed %6:gpr32
  13	%8:fpr64 = FMULDrr %7:fpr64, %7:fpr64
  18	%9:fpr64 = FADDDrr %0:fpr64, killed %8:fpr64
  23	%d0 = COPY %9:fpr64
  23	RET_ReallyLR implicit %d0
  Heights for %bb.1:
        6 Instructions
       1c @ A57UnitB (1 ops x6)
       1c @ A57UnitI (1 ops x3)
       1c @ A57UnitL (1 ops x6)
       1c @ A57UnitM (1 ops x6)
       2c @ A57UnitV (3 ops x3)
  23	0	RET_ReallyLR implicit %d0
  23	0	%d0 = COPY %9:fpr64
  23	5	%9:fpr64 = FADDDrr %0:fpr64, killed %8:fpr64
  23	10	%8:fpr64 = FMULDrr %7:fpr64, %7:fpr64
  23	20	%7:fpr64 = SCVTFUWDri killed %6:gpr32
  23	23	%6:gpr32 = MADDWrrr %1:gpr32, %2:gpr32, %3:gpr32
  %bb.1 Live-ins: %0 at 5 %1 at 23 %2 at 23 %3 at 23
  Critical path: 23
    Dependence data for %9:fpr64 = FADDDrr %0:fpr64, killed %8:fpr64
  	NewRootDepth: 13	RootDepth: 18
  	NewRootLatency: 9	RootLatency: 10
  	RootSlack: 0 SlackIsAccurate=1
  	NewRootDepth + NewRootLatency = 22
  	RootDepth + RootLatency + RootSlack = 28
  	  It IMPROVES PathLen because
  		NewCycleCount = 22, OldCycleCount = 28
  		Resource length before replacement: 5 and after: 4
  		  As result it IMPROVES/PRESERVES Resource Length
  Invalidate %bb.1 MinInstr height.
  Invalidate %bb.1 MinInstr depth.
  Combining MBB

================
Comment at: lib/CodeGen/MachineCombiner.cpp:478

+    DEBUG(dbgs() << " Possible instr(s) to replace\n");
+    DEBUG(dbgs() << "\t" << STI.getSchedInfoStr(MI) << ": ";
----------------
avt77 wrote:
> Gerolf wrote:
> > Duplicate info to what is displayed in the loops below?
> No, here we have only one possible instr to remove (the message should be changed). Below we could have a set of instrs to remove.
I think " Possible instr to replace" is not quite accurate. At this point we found > 0 patterns that could replace a pattern with `MI` as root.

================
Comment at: lib/CodeGen/MachineCombiner.cpp:437
-
-    DEBUG(dbgs() << "INSTR "; MI.dump(); dbgs() << "\n";);
     SmallVector<MachineCombinerPattern, 16> Patterns;
----------------
Why did you drop this debug message? It seems helpful to know which instructions the machine combiner already visited. Although I can also see why one could argue why dropping it is ok, as we have the reference in which basic block we are at through the debug message at the beginning of the function

================
Comment at: lib/CodeGen/MachineCombiner.cpp:478
+    DEBUG(dbgs() << " Possible instr to replace\n");
+    DEBUG(dbgs() << "\t" << STI->getSchedInfoStr(MI) << ": ";
+          MI.print(dbgs(), false, false, TII););
----------------
What additional information does the SchedInfoStr add here? 

================
Comment at: lib/CodeGen/MachineCombiner.cpp:479
+    DEBUG(dbgs() << "\t" << STI->getSchedInfoStr(MI) << ": ";
+          MI.print(dbgs(), false, false, TII););
+
----------------
I think we need a `\n` after print().

================
Comment at: lib/CodeGen/MachineInstr.cpp:1216
+      if (!TII)
+        TII = MF->getSubtarget().getInstrInfo();
+    }
----------------
The changes to `MachineInstr.cpp` seem unrelated to improving the debug output for the MachineCombiner? 

https://reviews.llvm.org/D41278