[StatepointsForGC] Identify values for rematerialization in presence of PHI

[anna]

Summary

While walking the use chain for identifying rematerializable values in RS4GC,
add the case where the current value and base value are the same PHI nodes.

This will aid rematerialization of geps and casts instead of adding gc.relocate.

[igor-laevsky]

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

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1816–1817 ↗

(On Diff #69357)

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.

1820 ↗

(On Diff #69357)

This should be named PhiNum according to the llvm convensions

1824–1826 ↗

(On Diff #69357)

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)

[anna]

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1824–1826 ↗

(On Diff #69357)

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.

[igor-laevsky]

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1824–1826 ↗

(On Diff #69357)

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.

[anna]

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

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1824–1826 ↗

(On Diff #69357)

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 :)

[anna]

Added a test case to show what we are trying to solve.
Also, consider reordering of arguments in phi, and consider incoming basic blocks as well.

Frankly, the best solution would be to remove the extra base.phi that is getting created, but need to see how to go about it.
I think in the current form, where base pointers that are phi nodes have an extra 'phi.base' getting generated,
we would not need the extra checks to make sure that the incoming basic blocks are the same, and also, the arguments won't be reordered.
These would be beneficial once we can remove the extra base.phi that get created.

[igor-laevsky]

looks good.

[reames]

LGTM w/minor comment

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1802 ↗

(On Diff #69438)

I think it's important to call out in the comment that this is a workaround for deficiency in the base pointer rewriting code. That's not immediately obvious to everyone reading. :)

We really need to rewrite that code at some point. My initial prototype version has lived far longer than it should have.

[anna]

In D23920#527833, @reames wrote:

LGTM w/minor comment

Ah, didnt see the update before my commit. However, I had added comments saying how such PHIs are generated and from which part of the algorithm. I'll explicitly mention this is a workaround for the deficiency. Thanks!

llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

static bool findRematerializableChainToBasePointer(
   SmallVectorImpl<Instruction*> &ChainToBase,
   Value *CurrentValue, Value *BaseValue) {
 
   // We have found a base value
   if (CurrentValue == BaseValue) {
     return true;
   }
 
+  // Check for same values, when both BaseValue and CurrentValue are phi nodes.
+  // PHI nodes that have the same incoming values, and belonging to the same
+  // basic blocks are essentially the same SSA value.  Such an example of same
+  // BaseValue, CurrentValue phis is created by findBasePointer, when a phi has
+  // incoming values with different base pointers. This phi is marked as
+  // conflict, and hence an additional phi with the same incoming values get
+  // generated. We need to identify the BaseValue (.base version of phi) and
+  // CurrentValue (the phi node itself) as the same, so that we can
+  // rematerialize the gep and casts below.
+  if (PHINode *CurrentPhi = dyn_cast<PHINode>(CurrentValue))
+    if (PHINode *BasePhi = dyn_cast<PHINode>(BaseValue)) {
+      auto PhiNum = CurrentPhi->getNumIncomingValues();
+      if (PhiNum != BasePhi->getNumIncomingValues() ||
+          CurrentPhi->getParent() != BasePhi->getParent())
+        return false;
+      // Map of incoming values and their corresponding basic blocks of
+      // CurrentPhi.
+      SmallDenseMap<Value *, BasicBlock *, 8> CurrentIncomingValues;
+      for (unsigned i = 0; i < PhiNum; i++)
+        CurrentIncomingValues[CurrentPhi->getIncomingValue(i)] =
+            CurrentPhi->getIncomingBlock(i);
+
+      // Both current and base PHIs should have same incoming values and
+      // the same basic blocks corresponding to the incoming values.
+      for (unsigned i = 0; i < PhiNum; i++) {
+        auto CIVI = CurrentIncomingValues.find(BasePhi->getIncomingValue(i));
+        if (CIVI == CurrentIncomingValues.end())
+          return false;
+        BasicBlock *CurrentIncomingBB = CIVI->second;
+        if (CurrentIncomingBB != BasePhi->getIncomingBlock(i))
+          return false;
+      }
+      return true;
+    }
+
   if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(CurrentValue)) {
     ChainToBase.push_back(GEP);
     return findRematerializableChainToBasePointer(ChainToBase,
                                                   GEP->getPointerOperand(),
                                                   BaseValue);
   }
 
   if (CastInst *CI = dyn_cast<CastInst>(CurrentValue)) {

llvm/trunk/test/Transforms/RewriteStatepointsForGC/rematerialize-derived-pointers.ll

entry:
   call void @do_safepoint() [ "deopt"() ]
 ; CHECK: gc.relocate
 ; CHECK: bitcast
 ; CHECK: gc.relocate
 ; CHECK: bitcast
   call void @use_obj32(i32 addrspace(1)* %ptr.gep11)
   ret void
 }
+
+
+declare i32 addrspace(1)* @new_instance() nounwind "gc-leaf-function"
+
+; remat the gep in presence of base pointer which is a phi node.
+; FIXME: We should remove the extra basephi.base as well.
+define void @contains_basephi(i1 %cond) gc "statepoint-example" {
+; CHECK-LABEL: contains_basephi
+entry:
+  %base1 = call i32 addrspace(1)* @new_instance()
+  %base2 = call i32 addrspace(1)* @new_instance()
+  br i1 %cond, label %here, label %there
+
+here:
+  br label %merge
+
+there:
+  br label %merge
+
+merge:
+  ; CHECK: %basephi.base = phi i32 addrspace(1)* [ %base1, %here ], [ %base2, %there ], !is_base_value !0
+  ; CHECK: %basephi = phi i32 addrspace(1)* [ %base1, %here ], [ %base2, %there ]
+  ; CHECK: %ptr.gep = getelementptr i32, i32 addrspace(1)* %basephi, i32 15
+  ; CHECK: %statepoint_token = call token (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint
+  ; CHECK: %basephi.base.relocated = call coldcc i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(token %statepoint_token, i32 7, i32 7) ; (%basephi.base, %basephi.base)
+  ; CHECK: %basephi.base.relocated.casted = bitcast i8 addrspace(1)* %basephi.base.relocated to i32 addrspace(1)*
+  ; CHECK: %ptr.gep.remat = getelementptr i32, i32 addrspace(1)* %basephi, i32 15
+  ; CHECK: call void @use_obj32(i32 addrspace(1)* %ptr.gep.remat)
+
+
+
+  %basephi = phi i32 addrspace(1)* [ %base1, %here ], [ %base2, %there ]
+  %ptr.gep = getelementptr i32, i32 addrspace(1)* %basephi, i32 15
+  call void @do_safepoint() ["deopt"() ]
+  call void @use_obj32(i32 addrspace(1)* %ptr.gep)
+  ret void
+}