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

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added inline comments.


================
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.
----------------
bogner wrote:
> Maybe move this comment up so it's a bit clearer that the sizes matching imply we've already built the index table?
Sure, will improve comments around here!


================
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?");
----------------
bogner wrote:
> range-for would be a bit more concise.
will do


================
Comment at: lib/IR/AsmWriter.cpp:511-512
+  }
+  assert(NumberedTypes.size() == Type2Number.size() &&
+         "Didn't get a unique numbering?");
+  return NumberedTypes;
----------------
bogner wrote:
> AFAICT this assert and the one inside the loop are checking the same thing. Do we need them both?
Not quite, but I'm checking the uniqueness one totally wrong, I will fix it, good catch!


================
Comment at: lib/IR/AsmWriter.cpp:526
+  NamedTypes.run(*M, false);
+  M = nullptr;
 
----------------
bogner wrote:
> 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?
Sure, will do, thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D44132