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

[Philip Reames via llvm-commits]

reames 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
----------------
anna wrote:
> 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?
> 
> 
> 
> 
> 
If I've read this correctly, I think the root issue here is that there's an undocumented assumption in the rematerialization code that a live derived pointer (which we must have if we're remating) must also keep it's base pointer live over the use site.  

The problem (as you said) is that we're essentially inserting a use of a value which *isn't* live over the call.  There's an equivalent version which *is*, but the original phi isn't.

I can see why your fix papers over the problem, but I'm concerned that it's not a root fix.  In particular, how this interacts with multiple calls between the def and the use is unclear.  I need to think through that further.   I'll update again in the near future.


https://reviews.llvm.org/D24399