[PATCH] D23920: [StatepointsForGC] Identify PHI values for rematerialization

[Igor Laevsky via llvm-commits]

igor-laevsky added inline comments.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1824-1826
@@ +1823,5 @@
+        return false;
+      for (unsigned i = 0; i < phiNum; i++)
+        if (PhiCurr->getIncomingValue(i) != PhiBase->getIncomingValue(i))
+          return false;
+      return true;
----------------
anna wrote:
> igor-laevsky wrote:
> > I think you should check that incoming blocks are equivalent. In theory we can have two phi nodes with similar incoming values which are assigned to the different basic blocks, i.e:
> >   base_value    = phi (%a, BB1), (%b, BB2)
> >   current_value = phi (%a, BB2), (%b, BB1)
> > 
> > Also we can have two equivalent phi nodes with values recorder in a different order, i.e:
> >   base_value    = phi (%a, BB1), (%b, BB2)
> >   current_value = phi (%b, BB2), (%a, BB1)
> > 
> IIUC, base_value != current_value for the first example?
> ```
> base_value    = phi (%a, BB1), (%b, BB2)
> current_value = phi (%a, BB2), (%b, BB1)
> ```
> Although the value is coming from different basic blocks, since the incoming value is an SSA value, `(%a, BB1)` is same as `(%a, BB2)`. There is only one definition for `%a`, and all uses are dominated by the def. So, `base_value` == `current_value` in the example. 
> Another concern that came to mind as I saw the example was, if we needed a recursive check if `%a` was also a phi node. Again, we do not need this check, for the same reason that `%a` is an SSA value.
> 
> Regarding the second example, I initially had this in mind while writing the patch, but the check complexity is O(n log n) for n = `PhiNum`. Didnt think it warranted the expense :) I'll add this case.
I still think that base_value != current_value at runtime. See extended example:

```
BB1:
  br %merge

BB2:
  br %merge

merge:
  base_value    = phi (%a, BB1), (%b, BB2)
  current_value = phi (%b, BB1), (%a, BB2)
```

Say at runtime control flow will go through BB1. This would mean that "base_value == %a" and "current_value == %b". In case if we go through BB2, situation will reverse: "base_value == %b" and "current_value == %a". This means that at runtime base_value will never be equal to the current_value.

I suspect this situation is hardly possible on practice, since we know that base_value is a base pointer for the current_value. Yet I'm not convinced that we can simply ignore this problem.


https://reviews.llvm.org/D23920