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

[Anna Thomas via llvm-commits]

I’m not sure why the web updates are not reflecting in the email. Updated the thread about an hour back through phab:
https://reviews.llvm.org/D23920



I think base_value and current_value are the same in the example below:

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.

On Aug 26, 2016, at 10:48 AM, Igor Laevsky <igor at azulsystems.com<mailto:igor at azulsystems.com>> wrote:

igor-laevsky requested changes to this revision.
igor-laevsky added a comment.
This revision now requires changes to proceed.

Could you please add a couple of tests? Also you should update comment in the header of the "findRematerializableChainToBasePointer".


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1816-1817
@@ -1815,1 +1815,4 @@

+  // PHI nodes that have the same incoming values, and belonging to the same
+  // basic blocks are essentially the same SSA value.
+  if (PHINode *PhiCurr = dyn_cast<PHINode>(CurrentValue))
----------------
I would emphasise in the comment that it's a termination condition, not a continuation of recursion. Also would be nice to move it closer to the "CurrentValue == BaseValue" case.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1820
@@ +1819,3 @@
+    if (PHINode *PhiBase = dyn_cast<PHINode>(BaseValue)) {
+      int phiNum = PhiCurr->getNumIncomingValues();
+      if (phiNum != PhiBase->getNumIncomingValues() ||
----------------
This should be named PhiNum according to the llvm convensions

================
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;
----------------
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)



https://reviews.llvm.org/D23920




-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160826/5893e6ea/attachment.html>