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

[Philip Reames via llvm-commits]

reames requested changes to this revision.
This revision now requires changes to proceed.

================
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:
> 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.
Ok, what do you think of the following restructuring:
- findRematerializableChainToBasePointer can be changed to return the root of the chain.  Where we'd today return either true or false, we now return the root of the chain instead.
- move the PHI matching logic into the caller by inspecting the root return value.  This includes an assertion that the alternate root is in the liveset for the call.  
- change the rematerializeChain function to explicitly replace uses of the original root with the updated root.  This avoids the replaceAllUsesOf which worries me.  




https://reviews.llvm.org/D24399