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

[Anna Thomas via llvm-commits]

anna marked 2 inline comments as done.

================
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:
> 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.


https://reviews.llvm.org/D23920