[PATCH] D41278: NFC: The improved debug logging for Machine Combiner

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a reviewer: Gerolf.
fhahn added a comment.

Thanks. I've left some minor comments. Could you add the debug output created for an example? That would be helpful to get a better picture on the new format.

Also adding Gerolf as reviewer.



================
Comment at: lib/CodeGen/MachineCombiner.cpp:19
 #include "llvm/CodeGen/MachineFunctionPass.h"
+#include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineLoopInfo.h"
----------------
What is this needed for?


================
Comment at: lib/CodeGen/MachineCombiner.cpp:140
 
+  const TargetSubtargetInfo &STI = MF->getSubtarget();
+
----------------
I think we should wrap that in `DEBUG`.


================
Comment at: lib/CodeGen/MachineCombiner.cpp:436
   DEBUG(dbgs() << "Combining MBB " << MBB->getName() << "\n");
+  const TargetSubtargetInfo &STI = MF->getSubtarget();
 
----------------
I think we should wrap that in `DEBUG`.


================
Comment at: lib/CodeGen/MachineCombiner.cpp:499
 
+      DEBUG(dbgs() << "\tDelInstrs\n");
+      for (auto *InstrPtr : DelInstrs) {
----------------
I think mentioning that for the current matched pattern, those are the instructions going to be removed and inserted instead would be clearer.


================
Comment at: lib/CodeGen/MachineCombiner.cpp:500
+      DEBUG(dbgs() << "\tDelInstrs\n");
+      for (auto *InstrPtr : DelInstrs) {
+        DEBUG(dbgs() << "\t\t" << STI.getSchedInfoStr(*InstrPtr) << ": ";
----------------
const pointer?


================
Comment at: lib/CodeGen/MachineCombiner.cpp:505
+      DEBUG(dbgs() << "\tInsInstrs\n");
+      for (auto *InstrPtr : InsInstrs) {
+        DEBUG(dbgs() << "\t\t" << STI.getSchedInfoStr(*InstrPtr) << ": ";
----------------
const pointer?


================
Comment at: lib/CodeGen/MachineCombiner.cpp:571
 bool MachineCombiner::runOnMachineFunction(MachineFunction &MF) {
+  this->MF = &MF;
   const TargetSubtargetInfo &STI = MF.getSubtarget();
----------------
Usually we do not use `this->`.


https://reviews.llvm.org/D41278