[PATCH] D66012: [AArch64][Statepoints] Statepoint support for AArch64.

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a reviewer: sanjoy.
kristof.beyls added a comment.

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.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:816-819
+  // Handle STACKMAP, PATCHPOINT and STATEPOINT specially and then use the
+  // generic code.
+  if (Opc == TargetOpcode::STACKMAP || Opc == TargetOpcode::PATCHPOINT ||
+      Opc == TargetOpcode::STATEPOINT) {
----------------
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).


================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:445-447
     OutStreamer->EmitAssemblerFlag(MCAF_SubsectionsViaSymbols);
-    emitStackMaps(SM);
   }
+  emitStackMaps(SM);
----------------
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66012/new/

https://reviews.llvm.org/D66012