[PATCH] D40885: [NFC] Refactor SafepointIRVerifier

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:140
 
+using AvailableValueSet = DenseSet<const Value *>;
+
----------------
Please, check it in as separate NFC. This is a big patch, and it is hard to follow what is going on while you're mixing refactoring and functional changes.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:241
+class GCPtrTracker {
+  const Function &F;
+  const DominatorTree &DT;
----------------
Do you really need this field? It is only used in `verifyFunction`, why not just pass the function into it as parameter?


================
Comment at: lib/IR/SafepointIRVerifier.cpp:245
+  DenseMap<const BasicBlock *, BasicBlockState *> BlockMap;
+  // This set contains defs that can be safely ignored during verification.
+  DenseSet<const Instruction *> ValidUnrelocatedDefs;
----------------
Please, give it more explanation what do you mean by "can be safely ignored".


================
Comment at: lib/IR/SafepointIRVerifier.cpp:365
+    for (const Instruction &I : *BB) {
+      if (Tracker.instructionMayBeSkipped(&I)) {
+        continue; // This instruction shouldn't be added to AvailableSet.
----------------
{} not needed


================
Comment at: lib/IR/SafepointIRVerifier.cpp:367
+        continue; // This instruction shouldn't be added to AvailableSet.
+      } else {
+        Verifier.verifyInstruction(&Tracker, I, AvailableSet);
----------------
Else is not needed.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:372
+      bool Cleared = false;
+      transferInstruction(I, Cleared, AvailableSet);
+      (void)Cleared;
----------------
Could you please add a comment what is going on here?


================
Comment at: lib/IR/SafepointIRVerifier.cpp:412
 
-static void Verify(const Function &F, const DominatorTree &DT) {
-  SpecificBumpPtrAllocator<BasicBlockState> BSAllocator;
-  BlockStateMap BlockMap;
-
-  DEBUG(dbgs() << "Verifying gc pointers in function: " << F.getName() << "\n");
-  if (PrintOnly)
-    dbgs() << "Verifying gc pointers in function: " << F.getName() << "\n";
-
-
-  for (const BasicBlock &BB : F) {
-    BasicBlockState *BBS = new(BSAllocator.Allocate()) BasicBlockState;
-    for (const auto &I : BB)
-      TransferInstruction(I, BBS->Cleared, BBS->Contribution);
-    BlockMap[&BB] = BBS;
+bool GCPtrTracker::leaveOnlyRelocatedDefs(const BasicBlock *BB,
+                                          const BasicBlockState *BBS,
----------------
How about `removeUnrelocatedDefs`? It would be more consistent with what debug message in this function is.


https://reviews.llvm.org/D40885