[PATCH] D40289: [SafepointIRVerifier] Allow deriving pointers from unrelocated base

[Daniil Suchkov via Phabricator via llvm-commits]

DaniilSuchkov marked 10 inline comments as done.
DaniilSuchkov added inline comments.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:311
+  // TODO: It would be better if worklist will work as a queue (not as a stack).
+  // This way this algorithm will converge faster.
   SetVector<const BasicBlock *> Worklist;
----------------
mkazantsev wrote:
> It's not clear to me why. I see how topological order of successors gives us gain, but it's not clear why we need BFS in addition to that. Could you please give an example?
Sorry, my comment wasn't clear enough.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:336
     if (OldOutCount != BBS->AvailableOut.size()) {
-      assert(OldOutCount > BBS->AvailableOut.size() && "invariant!");
+      assert(OldOutCount >= BBS->AvailableOut.size() && "invariant!");
+      // TODO: It is better to add successors to a worklist in topological order
----------------
mkazantsev wrote:
> What was the reason of changing `>` to `>=`? They can never be equal due to condition one line above.
This assertion was supposed to be before the if-statement.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:421
+        ValidUnrelocatedDefs.insert(&I);
+        DEBUG(dbgs() << "Removing " << I << " from Contribution of "
+                     << BB->getName() << "\n");
----------------
mkazantsev wrote:
> Should be ` << *I <<` if you want a reasonable output.
`I` is a reference, so it's OK.


https://reviews.llvm.org/D40289