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

[Anna Thomas via Phabricator via llvm-commits]

anna requested changes to this revision.
anna added a comment.
This revision now requires changes to proceed.

Some refactoring comments in place. I think the code structuring can be made much simpler. Pls see inline comments.



================
Comment at: lib/IR/SafepointIRVerifier.cpp:300
+/// It calls Visitor for each visited BB after updating it's AvailableIn.
+/// Visitor may change BB's Contribution and should return true it this case.
+///
----------------
Nit: Stale comment. It's `BBContributionUpdater`


================
Comment at: lib/IR/SafepointIRVerifier.cpp:303
+/// BBContributionUpdater is expected to have following signature:
+/// (const BasicBlock *BB, const BasicBlockState *BBS,
+///  DenseSet<const Value *> &Contribution) -> bool
----------------
If `BBContributionUpdater` is expected to have this type, then why templatize it?
You could just typedef instead of using templated type right, I think it makes the intent clearer.

<This comment is superseded by refactoring comment below>


================
Comment at: lib/IR/SafepointIRVerifier.cpp:330
+    bool InputsChanged = OldInCount != BBS->AvailableIn.size();
+    bool ContributionChanged =
+        BBContributionUpdater(BB, BBS, BBS->Contribution);
----------------
Instead of passing in an entire lambda, why not just use a static function `BBContributionUpdater` that also takes in a bool, which identifies if this is "after computing def states" or if this is the initial stage (i.e. `BBContributionUpdater` just return false if it was the initial stage) ?

It avoids all this template and function pointers and identifying what's going on. Also all logic is in one place. 


https://reviews.llvm.org/D40289