[PATCH] D41006: [SafepointIRVerifier] Allow non-dereferencing uses of unrelocated or poisoned PHI nodes

[Anna Thomas via Phabricator via llvm-commits]

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

Logic looks right, some comments inline.



================
Comment at: lib/IR/SafepointIRVerifier.cpp:409
 
 bool GCPtrTracker::instructionMayBeSkipped(const Instruction *I) const {
+  return ValidUnrelocatedDefs.count(I) || PoisonedDefs.count(I);
----------------
Pls add a comment here on why the poisoned defs are skipped. `ValidUnrelocatedDefs` are obvious.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:481
+    bool ValidUnrelocatedPointerDef = false;
+    bool PoisonedPointerDef = false;
+    if (const PHINode *PN = dyn_cast<PHINode>(&I)) {
----------------
Don't we need to handle selects and identify poisoned versus unrelocated?


================
Comment at: lib/IR/SafepointIRVerifier.cpp:491
+
+          if (isNotExclusivelyConstantDerived(InValue)) {
+            if (isValuePoisoned(InValue)) {
----------------
Instead of the below code (logic is right, but too many conditionals), could we do something like this:
```
if (isNotExclusivelyConstantDerived(InValue)) {
  if (isValuePoisoned(InValue) || (HasRelocatedInputs &&  HasUnrelocatedInputs)) {
     PoisonedPointerDef = true;
     break;
   }
   if (BlockMap[InBB]->AvailableOut.count(InValue))
              HasRelocatedInputs = true;
    else
              HasUnrelocatedInputs = true;
}
if (!PoisonedPointerDef && HasUnrelocatedInputs) {
   assert(!HasRelocatedInputs && "Should be poisoned!");
   ValidUnrelocatedPointerDef = true;
}
```



https://reviews.llvm.org/D41006