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

[Anna Thomas via llvm-commits]

anna added a comment.

Upcoming change to handle review comments and with test cases from these examples discussed in review.


================
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;
----------------
igor-laevsky wrote:
> 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.
I see your point. I was looking at the incoming data values statically, rather than the control flow at runtime. It would take exactly one path :)




https://reviews.llvm.org/D23920