Page MenuHomePhabricator

[AArch64][Statepoints] Statepoint support for AArch64.
Needs ReviewPublic

Authored by loicottet on Aug 9 2019, 7:19 AM.

Details

Summary

Provides support for garbage collection statepoints in the AArch64 backend. The GC transition flag is not supported yet.

Diff Detail

Event Timeline

loicottet created this revision.Aug 9 2019, 7:19 AM
loicottet updated this revision to Diff 214790.Aug 13 2019, 2:53 AM

Correction of failing test cases

phosek added a subscriber: phosek.Aug 19 2019, 1:57 PM
loicottet retitled this revision from Statepoint support for AArch64 to [AArch64][Statepoints] Statepoint support for AArch64.Aug 21 2019, 7:13 AM
loicottet retitled this revision from [AArch64][Statepoints] Statepoint support for AArch64 to [AArch64][Statepoints] Statepoint support for AArch64..
loicottet updated this revision to Diff 216835.Aug 23 2019, 7:02 AM

Style changes

I'm afraid I'm not up to speed on statepoints, but I thought I'd give some feedback nonetheless on the code as is.
I hope it's useful.

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
816–819 ↗(On Diff #216835)

It seems strange to me that adding AArch64-target specific support (the purpose of this patch, I presume) needs a generic target-independent change like this.
If this is a bug in the target-independent part of statepoint support, I think it'd be best to split this off as a separate patch, with a regression tests demonstrating this issue on a target that already supports statepoints (which I presume is only x86_64 today).

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
445–448

If I understand the semantics of this change, the goal is to emit stack maps not only for MachO targets, but for all targets, such as those using ELF or COFF.
This implies that stack maps are not supported for ELF AArch64. According to documentation at https://llvm.org/docs/StackMaps.html stackmaps are also needed for supporting stackmap and patchpoint intrinsics.
In the spirit of incremental development, I wonder if it'd be possible to again split this off as a separate patch, introducing stackmap support?
I guess to do that, also support for patchpoint and stackmap intrinsics for ELF (and COFF?) targeting AArch64 may be needed, which may be non-trivial? OTOH, it seems those are already supported for MachO AArch64, so maybe it'd be a relatively small/simple thing to do in a separate patch?
Do you intend to also support COFF, or only add support for ELF here? If so, the emitStackMaps may still need to be guarded by an if-condition?

sanjoy edited reviewers, added: apilipenko; removed: sanjoy.Sep 18 2019, 1:44 PM
sanjoy added a subscriber: sanjoy.Sep 18 2019, 1:51 PM

I don't work on statepoints anymore.

reames requested changes to this revision.Sun, Oct 20, 6:28 PM
reames added inline comments.
llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
816–819 ↗(On Diff #216835)

This does not look correct to me. I'd definitely demand to see a separate patch with a clear test case justifying this.

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
445–448

I agree on the separable patch bit. I think this is pretty straight forward, but a simple test and one line patch would be good.

779

This is wrong. STACKMAP and PATCHPOINT are destructive patching. STATEPOINT assumes non-destuctive patching. (i.e. no shadow region, nops explicitly supported and likely replaced before the code ever runs)

This revision now requires changes to proceed.Sun, Oct 20, 6:28 PM
loicottet marked 3 inline comments as done.Fri, Nov 8, 7:11 AM
loicottet added inline comments.
llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
816–819 ↗(On Diff #216835)

This has indeed nothing to do here. I removed it.

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
445–448

I've been able to use statepoint and stackmap intrinsics and emit stack maps using these changes without trouble (for ELF at least). From the git history, it appears that the stack map was originally emitted for all object file formats until the MachO hack was added in be1d1b66. It looks like the stack map emission was included in it by mistake. I'll open a separate patch to fix this.

779

Reverted. I misunderstood the patching behaviours of the intrinsics.

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
2227–2241

Should this bugfix be part of a separate patch or is it ok to leave it in this PR as it enables correct statepoint operation?

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
185–188

Not sure about this, I'd appreciate if someone could confirm this is the way to do it.

loicottet updated this revision to Diff 228447.Fri, Nov 8, 7:13 AM

Addressed comments

loicottet marked an inline comment as done.Mon, Nov 11, 1:49 AM
loicottet added inline comments.
llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
445–448