This is an archive of the discontinued LLVM Phabricator instance.

[RS4GC] Fix crash in the case that a live variable has a constant base.
ClosedPublic

Authored by mjacob on Dec 15 2015, 7:48 PM.

Details

Summary

Previously, RS4GC crashed in CreateGCRelocates() because it assumed
that every base is also in the array of live variables, which isn't true if a
live variable has a constant base.

This change fixes the crash by making sure CreateGCRelocates() won't try to
relocate a live variable with a constant base. This would be unnecessary
anyway because anything with a constant base won't move.

Diff Detail

Event Timeline

mjacob updated this revision to Diff 42960.Dec 15 2015, 7:48 PM
mjacob retitled this revision from to [RS4GC] Fix crash in the case that a live variable has a constant base..
mjacob updated this object.
mjacob added a reviewer: reames.
mjacob added a subscriber: llvm-commits.
reames edited edge metadata.Dec 16 2015, 4:10 PM

Comments inline.

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
1620

I think the general approach of filtering the liveset here can work, but this is the wrong place for it. In particular, it's after the vector splitting and re-materialization logic. The vector splitting probably isn't an issue - I'm about to revise/remove that anyways - but the remat logic definitely has coupling with the relocation logic.

I think you probably should pull this out as a separate step between the recomputation of liveness after base pointer rewriting and the first use of the new liveness in splitVectorValues.

I'll also require much better comments. :) For instance, why does simply rewriting the live value as a constant-expr fail? (i.e. why is the right approach?)

p.s. A possible alternate approach would be to stop filtering constants from the liveset. In that case, we would have the live constant base in the liveset. Have you considered this option?

test/Transforms/RewriteStatepointsForGC/base-pointers-12.ll
11

instead of undef, use an i1 argument.

A valid canonicalizing transform (i.e. simplifyInstruction) would break this test in a non-obvious way.

14

Add a check for the load from select.

mjacob updated this revision to Diff 43193.Dec 17 2015, 3:28 PM
mjacob edited edge metadata.

Address review comments.

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
1620

Agreed, filterung the liveset earlier makes sense. I'll also add a comment.

I'm not sure what you mean by "rewriting the live value as a constant-expr". Please clarify if that's still relevant for my next diff.

Adding the constant base to the liveset would probably solve the issue, too. However that would make the liveset bigger and would still need filtering later, at least for my custom stack map format which doesn't support constants.

reames accepted this revision.Dec 17 2015, 7:23 PM
reames edited edge metadata.

LGTM w/minor comments addressed.

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
2390

Can you give a small example here? It would make it easier to understand. Possibly: "for instance, a variable offset into a constant global"

2392

and that the GC doesn't otherwise need to know about them.

p.s. Does your GC actually have constant references? Or are these just small offsets off of null? That's the only case we've seen to date.

2394

relies on this filtering for correctness as it expects...

test/Transforms/RewriteStatepointsForGC/base-pointers-12.ll
17

Can you add one or two other examples? For instance, a variable offset GEP from a global or a variable offset from a null pointer down a dead path?

This revision is now accepted and ready to land.Dec 17 2015, 7:23 PM
mjacob added inline comments.Dec 17 2015, 8:21 PM
lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
2392

Yes, my GC has constant references. GC objects in constants are immortal (they can't be deallocated), so they don't have to be reported by the statepoint mechanism. There is a separate mechanism that tracks references from immortal objects to potentially non-immortal objects.

test/Transforms/RewriteStatepointsForGC/base-pointers-12.ll
17

Should they go to another file?

Regarding "variable offset GEP from a global": the simple version of this with just the GEP live over the statepoint doesn't crash without the patch because of rematerialization. I can (a) add a more complex example which isn't handled by rematerialization or (b) check that it doesn't get rematerialized. I'm not sure which would be more useful.

Regarding "variable offset from a null pointer down a dead path": first, what exactly do you mean by "dead"? Statically dead paths are removed by RS4GC. Can you give an simple example of a dynamically, but not statically, dead path? I hope I use the correct terminology here. Also, a constant null pointer as a base will fire an assert assert(!isa<ConstantPointerNull>(base) ... in findBasePointers().

mjacob updated this revision to Diff 43376.Dec 21 2015, 8:18 AM
mjacob edited edge metadata.

Improve comment.

mjacob updated this revision to Diff 43378.Dec 21 2015, 8:29 AM

Improve comment.

mjacob updated this revision to Diff 43449.Dec 22 2015, 8:51 AM

Add another test.

mjacob closed this revision.Dec 22 2015, 8:54 AM

As this revision was already accepted, I committed it. Another test (Philip proposed a test "variable offset from a null pointer down a dead path") could be added in a follow-up commit after my open questions about this particular test case are answered.

reames added inline comments.Dec 22 2015, 10:33 AM
test/Transforms/RewriteStatepointsForGC/base-pointers-12.ll
17

You can put them in the same file or a different one as convient. No strong preference.

Sounds like we might want to consider adding a diagnostic flag to disable rematerialization. Doing so would make it easier to write the simple test and is probably worthwhile. Adding both the simple and complicated tests might be worth it as well.

By "dead", I meant "dynamically dead, but not statically dead". You can end up with all kinds of garbage down such a path because the compiler may have proven conflicting facts, but not yet realized the block is dead. Don't worry about this case specifically, I was mostly just throwing out ideas. I do want at least a bit more test coverage here, but I'm not picky about the form it takes.

p.s. The constant base assert has been removed.

mjacob added inline comments.Dec 22 2015, 2:36 PM
test/Transforms/RewriteStatepointsForGC/base-pointers-12.ll
17

OK, the commit includes another test case. I think testing that it won't rematerialize makes sense as well. So now we have one test with a non-materializable derived pointer and one with a materializable derived pointer.

I meant another assert which is still present. The comment says it's there because the verifier relies on it.