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

[Justin Bogner via Phabricator via llvm-commits]

bogner added inline comments.


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:143-144
+  for (auto *II : instructions) {
+    std::string s;
+    raw_string_ostream sstr(s);
+    II->print(sstr);
----------------
Capitalize variable names please (Call them S and OS, maybe).


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:148-150
+    const size_t i = sstr.str().find("=");
+    std::string str =
+        (i == std::string::npos) ? sstr.str() : sstr.str().substr(i);
----------------
Probably easier to understand / slightly more efficient to explicitly flush the stream and just use S, rather than calling str() repeatedly.


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:211-214
+  std::vector<MachineInstr *> pseudoIdempotentInstructions;
+  std::vector<MachineInstr *> mayStoreInstructions;
+  std::vector<MachineInstr *> mayLoadInstructions;
+  std::map<MachineInstr *, std::vector<MachineInstr *>> multiUsers;
----------------
More naming convention.


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:681
+  std::vector<unsigned> nonIdempotentDefs;
+  std::vector<MachineInstr *> pseudoIdempotentInstructions;
+  unsigned i = 0;
----------------
Unused?


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:682-683
+  std::vector<MachineInstr *> pseudoIdempotentInstructions;
+  unsigned i = 0;
+  unsigned ii = 0;
+  for (auto &MI : *MBB) {
----------------
These might need better names. It's very hard to scry what they're for.


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:686-687
+
+    if (pseudoIdempotentInstCount < 2)
+      break;
+
----------------
Maybe we should hoist this out of the loop, so it's obvious to readers that it isn't loop dependent? It might make sense to put the whole loop in its own function while we're at it.


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:689-698
+    if (i++ >= pseudoIdempotentInstCount) {
+      if (ii == 0) {
+        SkipVRegs(gap, MRI, DummyRC);
+      }
+
+      if (ii++ > 1)
+        break;
----------------
Some comments would make all of this easier to follow


Repository:
  rL LLVM

https://reviews.llvm.org/D44368