[PATCH] D24399: [RS4GC] Avoid usage of unrelocated values while rematerializing geps and casts

[Anna Thomas via llvm-commits]

anna added inline comments.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1859
@@ -1858,1 +1858,3 @@
       }
+      // The rematerialized gep/cast instructions should use the relocated base
+      // value (note that the current phi will never be relocated). So, all its
----------------
reames wrote:
> I'm not really following why this is needed.  How can there be uses of the original phi which don't get updated?  Isn't that a missed relocation bug?  Looking at the added test case, I don't see how that's related to the change?
> 
> If you could expand a bit more on the scenario which causes this and how we reach the problematic situation, I'd appreciate it.
Let's consider the first example:
```
%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
```

The findBasePointer function generates an additional phi for `basephi`:
```
 %basephi.base = phi i32 addrspace(1)* [ %base1, %here ], [ %base2, %there ], !is_base_value !0
```
The `ptr.gep` still has `basephi` as the pointer operand, but the `BaseValue` passed into `findRematerializableChainToBasePointer` for this gep is the `basephi.base` (newly added phi by `findBasePointer` function). 
We reach the point going up the use chain, where CurrentValue: `basephi` is same as BaseValue: `basephi.base`, both are the same phi nodes. 
We then rematerialize the `ptr.gep` instead of relocating it, but the code after RS4GC becomes:
```
%basephi = phi i32 addrspace(1)* [ %base1, %here ], [ %base2, %there ]
 %basephi.base.relocated = call coldcc i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(token %statepoint_token, i32 7, i32 7) ; 
%ptr.gep.remat = getelementptr i32, i32 addrspace(1)* %basephi, i32 15
```
The original `basephi` is never relocated, only `basephi.base` added by findBasePointer is relocated. However, the remat gep still points to the unrelocated value. This was what the original test case resulted in (left hand side of test1: `contains_basephi`). However, when passed through the ir-verifier, we can see that it's actually incorrect safepoint code.

P.S: The second test case is added to show that this problem never gets exposed when we relocate the gep. In this case too, basephi is never relocated, only the `basephi.base` and the `ptr.gep` gets relocated. Until now, this was not a problem since we never remat'ed geps/casts in the presence of this deficiency (creation of `basephi.base` in the `findBasePointer` algo).

I think the reason comes down to the fact that `basephi.base` is considered the BasePointer for relocation, since that is generated by `findBasePointer`. So, both the original fix and this new one for correct safepoint code, is to work around the `findBasePointer` deficiency. Should we go about this another way?







https://reviews.llvm.org/D24399