[PATCH] D44132: [MIR] Making MIR Printing, opt -dot-cfg, and -debug printing faster

[Justin Bogner via Phabricator via llvm-commits]

bogner accepted this revision.
bogner added a comment.
This revision is now accepted and ready to land.

Nice improvement! I have a couple of minor style/clarity comments, but LGTM with those addressed.



================
Comment at: lib/IR/AsmWriter.cpp:501-505
+  if (NumberedTypes.size() == Type2Number.size())
+    return NumberedTypes;
+
+  // We know all the numbers that each type is used and we know that it is a
+  // dense assignment.  Convert the map to an index table.
----------------
Maybe move this comment up so it's a bit clearer that the sizes matching imply we've already built the index table?


================
Comment at: lib/IR/AsmWriter.cpp:507
+  NumberedTypes.resize(Type2Number.size());
+  for (auto I = Type2Number.begin(), E = Type2Number.end(); I != E; ++I) {
+    assert(I->second < NumberedTypes.size() && "Didn't get a dense numbering?");
----------------
range-for would be a bit more concise.


================
Comment at: lib/IR/AsmWriter.cpp:511-512
+  }
+  assert(NumberedTypes.size() == Type2Number.size() &&
+         "Didn't get a unique numbering?");
+  return NumberedTypes;
----------------
AFAICT this assert and the one inside the loop are checking the same thing. Do we need them both?


================
Comment at: lib/IR/AsmWriter.cpp:526
+  NamedTypes.run(*M, false);
+  M = nullptr;
 
----------------
It's a bit surprising to null out the module to mark that we already ran this, but I suppose it's simpler than keeping an extra bool around just for that. Maybe add comments to point out what's happening with M, both here and by the member declaration?


Repository:
  rL LLVM

https://reviews.llvm.org/D44132