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

[Daniil Suchkov via Phabricator via llvm-commits]

DaniilSuchkov added inline comments.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:409
 
 bool GCPtrTracker::instructionMayBeSkipped(const Instruction *I) const {
+  return ValidUnrelocatedDefs.count(I) || PoisonedDefs.count(I);
----------------
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.


================
Comment at: lib/IR/SafepointIRVerifier.cpp:481
+    bool ValidUnrelocatedPointerDef = false;
+    bool PoisonedPointerDef = false;
+    if (const PHINode *PN = dyn_cast<PHINode>(&I)) {
----------------
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).


================
Comment at: lib/IR/SafepointIRVerifier.cpp:491
+
+          if (isNotExclusivelyConstantDerived(InValue)) {
+            if (isValuePoisoned(InValue)) {
----------------
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.


https://reviews.llvm.org/D41006