This is an archive of the discontinued LLVM Phabricator instance.

[RS4GC] Avoid usage of unrelocated values while rematerializing geps and casts
ClosedPublic

Authored by anna on Sep 9 2016, 8:09 AM.

Details

Summary

When rematerializing for geps and casts in presence of phis, we need to make
sure that unrelocated values (the current Phi node) should never be used.
The relocation is only done for the base version of the phi, which is generated
by findBasePointers function.

This is a fix when IR verifier for safepoints identifies uses of unrelocated values,
which was generated due to commit rL279975.

Diff Detail

Event Timeline

anna updated this revision to Diff 70839.Sep 9 2016, 8:09 AM
anna retitled this revision from to [RS4GC] Avoid usage of unrelocated values while rematerializing geps and casts.
anna updated this object.
anna added reviewers: sanjoy, reames, igor-laevsky.
anna added a subscriber: llvm-commits.
reames added inline comments.Sep 9 2016, 12:00 PM
lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
1819

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.

anna added inline comments.Sep 9 2016, 12:35 PM
lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
1819

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?

reames added inline comments.Sep 20 2016, 10:13 AM
lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
1819

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.

reames requested changes to this revision.Sep 20 2016, 10:31 AM
reames edited edge metadata.
reames added inline comments.
lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
1819

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.
This revision now requires changes to proceed.Sep 20 2016, 10:31 AM
anna added inline comments.Sep 20 2016, 10:48 AM
lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
1819

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.

anna updated this revision to Diff 72041.Sep 21 2016, 7:22 AM
anna edited edge metadata.

The semantic change required to remat in presence of phi.
Only replace use in the remat chain, rather than replacing all uses with the live value.
This is rebased over the refactoring NFC change: https://reviews.llvm.org/D24780
Verified IR generated in tests with safepoint-verifier, and running some local tests.

Looks close to ready, one more update should do it.

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
1983

ReplaceUnrelocatedUse doesn't need to be a reference. And shouldn't be.

2006

Strengthen this assert to include the root value and the new root value.

2016

I don't think you actually need this variable. If you just check OldRoot != NewRoot, doesn't that give you the same property?

anna updated this revision to Diff 72080.Sep 21 2016, 10:00 AM
anna edited edge metadata.

Addressed the 3 comments above.
Tested the asserts.

reames accepted this revision.Sep 21 2016, 2:09 PM
reames edited edge metadata.

LGTM. Thanks!

This revision is now accepted and ready to land.Sep 21 2016, 2:09 PM
anna added a comment.Sep 22 2016, 6:21 AM

Thanks Philip! local tests results look fine (some are ongoing). I'll check this in.

anna closed this revision.Sep 22 2016, 7:09 AM

Committed as: https://reviews.llvm.org/rL282150

Forgot to amend the commit, so this did not auto-close the revision.