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

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:491
+
+          if (isNotExclusivelyConstantDerived(InValue)) {
+            if (isValuePoisoned(InValue)) {
----------------
anna wrote:
> 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.
also, pls point out as a comment where these rules come from (i.e. refer to header for reasoning of these rules).


https://reviews.llvm.org/D41006