[PATCH] D44368: MIR-Canon Idempotent Instruction Hoisting.

[Justin Bogner via Phabricator via llvm-commits]

bogner added inline comments.


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:195-196
+  for (auto *II : Instructions) {
+    for (unsigned i = 1; i < II->getNumOperands(); i++) {
+      MachineOperand &MO = II->getOperand(i);
+      if (!MO.isReg())
----------------
for (MachineOperand &MO : II->operands()) ?


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:670-674
+  // Here we renumber the def vregs for the idempotent instructions from the top
+  // of the MachineBasicBlock so that they are named in the order that we sorted
+  // them alphabetically. If there's less than two idempotent instructions we
+  // don't bother with this step because it doesn't seem to have a lot of value
+  // in real world cases we've tried.
----------------
What's the downside of doing this with less than two idempotent instructions? ISTM the simpler implementation of just doing it anyway would have some value.


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:678-679
+    unsigned i = 0;
+    unsigned gap = 1;
+    SkipVRegs(gap, MRI, DummyRC);
+
----------------
I'd probably skip the "gap" variable so it's obvious it's only used once. A named argument comment adds the same clarity: SkipVRegs(/*VRegGapIndex=*/1, MRI, DummyRC)


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:681-685
+    for (auto &MI : *MBB) {
+
+      if (i++ >= PseudoIdempotentInstCount) {
+        break;
+      }
----------------
It took me a minute to figure out that this was iterating over the first N instructions. Would it be clearer to use I as the loop variable here, like so?

  auto MII = MBB.begin();
  for (unsigned I = 0; I < PseudoIdempotentInstCount && MII != MBB.end(); ++I) {
    MachineInstr &MI = *MII++;

If you don't think that's better, at least move the initialization of I to right before the loop please.


Repository:
  rL LLVM

https://reviews.llvm.org/D44368