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

[Anna Thomas via Phabricator via llvm-commits]

anna accepted this revision.
anna added a comment.
This revision is now accepted and ready to land.

lgtm w/ comment addressed.



================
Comment at: lib/IR/SafepointIRVerifier.cpp:409
 
 bool GCPtrTracker::instructionMayBeSkipped(const Instruction *I) const {
+  return ValidUnrelocatedDefs.count(I) || PoisonedDefs.count(I);
----------------
DaniilSuchkov wrote:
> anna wrote:
> > Pls add a comment here on why the poisoned defs are skipped. `ValidUnrelocatedDefs` are obvious.
> You think I should repeat myself here? From comments above (where 'poisoned' pointers are introduced) it's clear why this defs are skipped.
Are all poisoned defs removed? Only valid ones right.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:481
+    bool ValidUnrelocatedPointerDef = false;
+    bool PoisonedPointerDef = false;
+    if (const PHINode *PN = dyn_cast<PHINode>(&I)) {
----------------
DaniilSuchkov wrote:
> anna wrote:
> > Don't we need to handle selects and identify poisoned versus unrelocated?
> I'll do it in the next patch (in order to keep this one not too huge).
pls add a TODO then.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:491
+
+          if (isNotExclusivelyConstantDerived(InValue)) {
+            if (isValuePoisoned(InValue)) {
----------------
DaniilSuchkov wrote:
> anna wrote:
> > 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;
> > }
> > ```
> > 
> But we still have to handle case when some inputs are relocated and some are not: your code won't work if the _last_ input makes this phi poisoned (because on each iteration it checks flags that might have changed on previous one). Thus we cannot remove this part: 
> ```
> if (HasUnrelocatedInputs) {
>   if (HasRelocatedInputs)
>     PoisonedPointerDef = true;
>   else
>     ValidUnrelocatedPointerDef = true;
> }
> ```
> So the only thing that might be changed is this part:
> ```
> if (isValuePoisoned(InValue)) {
>   // If any of inputs is poisoned, output is always poisoned too.
>   HasRelocatedInputs = true;
>   HasUnrelocatedInputs = true;
>   break;
> }
> ```
> But if we'll change it to
> ```
> if (isValuePoisoned(InValue)) {
>   // If any of inputs is poisoned, output is always poisoned too.
>   PoisonedPointerDef = true;
>   break;
> }
> ```
> We'll have to somehow tell that `if` (mentioned before) not to touch this flag and it'll be even worse.
> And that `assert` shouldn't be moved to PHI's branch, it's about all instructions.
> 
> Currently this part is pretty clear and straightforward: loop over phi's inputs sets two flags (that have clear meaning) and after that loop another two flags are changed accordingly.
ah yes, it wont work for the last case.


https://reviews.llvm.org/D41006