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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:309
+template <typename VisitorTy>
+static void RecalculateBBsStates(BlockStateMap &BlockMap, VisitorTy &&Visitor) {
+  // TODO: It would be better if worklist will work as a queue (not as a stack).
----------------
Please rename `Visitor` to something that makes sense in terms of what it is expected to do. I'd rather prefer something like `CalculateBlockContribution` or `CalculateBlockContributionVisitor`.


================
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;
----------------
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?


================
Comment at: lib/IR/SafepointIRVerifier.cpp:328
 
-    assert(OldInCount > BBS->AvailableIn.size() && "invariant!");
+    bool isInputsChanged = OldInCount != BBS->AvailableIn.size();
+    bool isContributionChanged = Visitor(BB, BBS, BBS->Contribution);
----------------
I'd propose `InputsChanged` and `ContributionChanged` respectively. If you want to keep these names, please capitalize 1st letter since these are variables.


================
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
----------------
What was the reason of changing `>` to `>=`? They can never be equal due to condition one line above.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:338
+      // TODO: It is better to add successors to a worklist in topological order
+      // (if any). It will decrease number of iterations required to converge.
       Worklist.insert(succ_begin(BB), succ_end(BB));
----------------
"if any" -> "if such order is defined"?


================
Comment at: lib/IR/SafepointIRVerifier.cpp:398
+    DenseSet<const Value *> AvailableSet = BBS->AvailableIn;
+    bool isContributionChanged = false;
+    for (const Instruction &I : *BB) {
----------------
same as above, I propose `ContributionChanged` or `IsContributionChanged`.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:400
+    for (const Instruction &I : *BB) {
+      bool isProducesUnrelocatedPointer = false;
+      if ((isa<GetElementPtrInst>(I) || isa<BitCastInst>(I)) &&
----------------
`ProducesUnrelocatedPointer`


================
Comment at: lib/IR/SafepointIRVerifier.cpp:418
+        // Remove Def of unrelocated pointer from Contribution of this BB and
+        // trigger update of all it's successors.
+        Contribution.erase(&I);
----------------
it's -> its


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


================
Comment at: lib/IR/SafepointIRVerifier.cpp:491
+      } else if (ValidUnrelocatedDefs.count(&I)) {
+        continue; // This instruction shouldn't be added to AvailableSet
       } else {
----------------
Finalize sentence.


https://reviews.llvm.org/D40289