This is an archive of the discontinued LLVM Phabricator instance.

[Statepoints] Support for "patchable" statepoints.
ClosedPublic

Authored by sanjoy on May 6 2015, 7:56 PM.

Details

Summary

This change adds two new parameters to the statepoint intrinsic, i64 id
and i32 num_patch_bytes. id gets propagated to the ID field
in the generated StackMap section. If the num_patch_bytes is
non-zero then the statepoint is lowered to num_patch_bytes bytes of
nops instead of a call (the spill and reload code remains unchanged).
A non-zero num_patch_bytes is useful in situations where a language
runtime requires complete control over how a call is lowered.

This change brings statepoints one step closer to patchpoints. With
some additional work (that is not part of this patch) it should be
possible to get rid of TargetOpcode::STATEPOINT altogether.

PlaceSafepoints generates statepoint wrappers with both id and
num_patch_bytes set to 0. This can be made more sophisticated
later.

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy updated this revision to Diff 25119.May 6 2015, 7:56 PM
sanjoy retitled this revision from to [Statepoints] Support for "patchable" statepoints..
sanjoy updated this object.
sanjoy edited the test plan for this revision. (Show Details)
sanjoy added a subscriber: Unknown Object (MLST).
sanjoy updated this object.May 11 2015, 4:12 PM
sanjoy updated this object.
sanjoy updated this object.May 11 2015, 4:15 PM
sanjoy updated this revision to Diff 25522.May 11 2015, 4:23 PM
  • rebase on master, now that D9501 has gone in
pgavlin added inline comments.May 11 2015, 4:40 PM
docs/Statepoints.rst
145 ↗(On Diff #25522)

Typo:

@llvm.experimental.gc.statepoint.p0f_isVoidfi64 0, i32 0, (void ()* @foo, i32 0

should be:

@llvm.experimental.gc.statepoint.p0f_isVoidf(void ()* @foo, i64 0, i32 0, i32 0

include/llvm/IR/Intrinsics.td
532 ↗(On Diff #25522)

Formatting is off here.

include/llvm/IR/Statepoint.h
92 ↗(On Diff #25522)

s/wit/with/

101 ↗(On Diff #25522)

An assertion that NumPatchBytesVal fits in 32 bits might be nice here.

lib/CodeGen/SelectionDAG/StatepointLowering.cpp
591–600 ↗(On Diff #25522)

It might be nice to move the definition of Ops and the subsequent pushes closer to their use (i.e. somewhere near line 650).

602 ↗(On Diff #25522)

Comment is stale.

673 ↗(On Diff #25522)

s/4/ImmutableStatepoint::FlagsPos/

lib/IR/Verifier.cpp
1510–1511 ↗(On Diff #25522)

Might be nice to have a check that NumPatchBytes fits in 32 bits. This condition should be satisfied by the intrinsic signature, but a little paranoia isn't necessarily bad. No strong feeling here.

1552 ↗(On Diff #25522)

No need to fix this now, necessarily, but it would be nice to replace these literals with the relevant constants from Statepoint.h.

sanjoy added inline comments.May 11 2015, 5:23 PM
docs/Statepoints.rst
145 ↗(On Diff #25522)

Fixed.

include/llvm/IR/Intrinsics.td
532 ↗(On Diff #25522)

Fixed. Somehow my editor inserted tabs here and I could not tell the difference visually.

include/llvm/IR/Statepoint.h
92 ↗(On Diff #25522)

Fixed.

101 ↗(On Diff #25522)

Fixed.

lib/CodeGen/SelectionDAG/StatepointLowering.cpp
591–600 ↗(On Diff #25522)

Done.

602 ↗(On Diff #25522)

Fixed.

673 ↗(On Diff #25522)

Changed to ISP.getFlags().

lib/IR/Verifier.cpp
1510–1511 ↗(On Diff #25522)

Agreed. I added an assert (not an Assert) for this.

1552 ↗(On Diff #25522)

Agreed; good idea, but should probably happen in a separate patch.

sanjoy updated this revision to Diff 25534.May 11 2015, 5:23 PM
  • fix test cases after rebasing on master after D9592
  • address Pat's review
pgavlin accepted this revision.May 11 2015, 5:29 PM
pgavlin edited edge metadata.

LGTM modulo the comments above.

docs/Statepoints.rst
145 ↗(On Diff #25534)

Still one extra ( before void ()* @foo, ...

lib/IR/Verifier.cpp
3410–3411 ↗(On Diff #25534)

An assertion that the result type remains a pointer would be helpful. This assert certainly helped me find and correct a few calls to gc.relocate that I hadn't fixed up when I was adjusting things for the GC transition change.

This revision is now accepted and ready to land.May 11 2015, 5:29 PM
sanjoy added inline comments.May 11 2015, 5:35 PM
docs/Statepoints.rst
145 ↗(On Diff #25534)

Thanks, will fix.

lib/IR/Verifier.cpp
3410–3411 ↗(On Diff #25534)

This came in via a different change by Chen, I'll ask him to take a look.

swaroop.sridhar accepted this revision.May 11 2015, 8:19 PM
swaroop.sridhar edited edge metadata.

I could not find a test-case for valid patchable-statepoints. How is the Nop emission tested?

Thanks.

chenli added inline comments.
lib/IR/Verifier.cpp
3410–3411 ↗(On Diff #25534)

I will submit a patch to assert gc_relocate returns a pointer type.

I could not find a test-case for valid patchable-statepoints. How is the Nop emission tested?

Thanks.

There is a new test case in test/CodeGen/X86/statepoint-call-lowering.ll.

sanjoy updated this revision to Diff 25552.May 11 2015, 11:05 PM
sanjoy edited edge metadata.
  • fix typo in Statepoints.rst
  • add a test case testing for correct lowering of the id parameter in statepoints
reames edited edge metadata.May 12 2015, 2:24 PM

LGTM pending minor documentation and style comments. No further review needed.

docs/Statepoints.rst
318 ↗(On Diff #25552)

I'd comment here that the meaning is up to the user. It's an opaque value to LLVM. You might also comment that it's not guaranteed to be unique due to code duplication.

321 ↗(On Diff #25552)

More detail is needed here. What are the semantics of this patchable section? Is it structured to be atomically patchable in any way?

329 ↗(On Diff #25552)

Merge w/previous paragraph.

lib/Target/X86/X86MCInstLower.cpp
815 ↗(On Diff #25552)

Just to be clear, this is not trying to implement the shadow behaviour of a patchpoint right? It's just raw patchable bytes? Please clarify this in the documentation.

lib/Transforms/Scalar/PlaceSafepoints.cpp
889 ↗(On Diff #25552)

I would mildly prefer you use the ID which was hard coded in the stackmap code before. No reason to change behaviour if it's not needed.

sanjoy added inline comments.May 12 2015, 4:55 PM
docs/Statepoints.rst
318 ↗(On Diff #25552)

Done.

321 ↗(On Diff #25552)

Done. I did not mention about the section begin atomically patchable since that seems somewhat tangential, but I've mentioned that there are no alignment guarantees.

329 ↗(On Diff #25552)

Done.

lib/Target/X86/X86MCInstLower.cpp
815 ↗(On Diff #25552)

Done.

lib/Transforms/Scalar/PlaceSafepoints.cpp
889 ↗(On Diff #25552)

Done.

This revision was automatically updated to reflect the committed changes.
dougk added a subscriber: dougk.May 13 2015, 9:35 AM

The assertion NumPatchBytes >= 0 is tautologically true because the type is 'uint'.

The assertion NumPatchBytes >= 0 is tautologically true because the type is 'uint'.

Thank you for catching that, I'll fix it.

REPOSITORY

rL LLVM

http://reviews.llvm.org/D9546

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/