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

[Igor Laevsky via llvm-commits]

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