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

Anna Thomas via llvm-commits llvm-commits at lists.llvm.org
Tue Sep 20 10:48:39 PDT 2016


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:
> 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.  
> 
> 
agree with the restructuring. I started with point 3 initially, but decided to go ahead with replaceAllUsesOf. I could not think of a reason this would be bad. Also, it could potentially reduce the number of times we hit the phi matching logic. 

As you mention, replaceAllUsesOf is a very widespread semantic change, versus just replacing the uses of original root that we found in the chain. Much less riskier.



https://reviews.llvm.org/D24399





More information about the llvm-commits mailing list