[PATCH] D14738: Reimplement discriminator assignment algorithm.

[Diego Novillo via llvm-commits]

dnovillo added a comment.

Thanks for this.  The new algorithm seems much more straightforward than what I wrote initially.


================
Comment at: lib/Transforms/Utils/AddDiscriminators.cpp:179
@@ -174,1 +178,3 @@
+
   for (BasicBlock &B : F) {
+    for (auto &I : B.getInstList()) {
----------------
Please describe at a high level how the algorithm finds location that need a discriminator.

================
Comment at: lib/Transforms/Utils/AddDiscriminators.cpp:188
@@ +187,3 @@
+      L.first = DIL->getFilename();
+      L.second = DIL->getLine();
+      auto R = LBM[L].insert(std::make_pair(&B, (Metadata *)NULL));
----------------
Prefer if this is expressed as L = std::make_pair(DIL->getFilename(), DIL->getLine());

================
Comment at: lib/Transforms/Utils/AddDiscriminators.cpp:189
@@ +188,3 @@
+      L.second = DIL->getLine();
+      auto R = LBM[L].insert(std::make_pair(&B, (Metadata *)NULL));
+      if (LBM[L].size() == 1)
----------------
s/NULL/nullptr/

================
Comment at: lib/Transforms/Utils/AddDiscriminators.cpp:190
@@ +189,3 @@
+      auto R = LBM[L].insert(std::make_pair(&B, (Metadata *)NULL));
+      if (LBM[L].size() == 1)
+        continue;
----------------
Could you cache the access to LBM[L] into a reference to avoid the double lookup?

================
Comment at: lib/Transforms/Utils/AddDiscriminators.cpp:192
@@ +191,3 @@
+        continue;
+      if (R.second) {
+        auto *Scope = DIL->getScope();
----------------
Here, it would be useful to add a comment along the lines of: "If we could insert a different block in the same location, a discriminator is needed to distinguish both instructions."

================
Comment at: lib/Transforms/Utils/AddDiscriminators.cpp:196
@@ +195,3 @@
+            Builder.createFile(DIL->getFilename(), Scope->getDirectory());
+        R.first->second = Builder.createLexicalBlockFile(
+            Scope, File, DIL->computeNewDiscriminator());
----------------
Could you create a reference to R.first->second and R.second?  It's easier to have a symbolic name associated to track what the code is doing. 


http://reviews.llvm.org/D14738