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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
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) {
----------------
kristof.beyls wrote:
> 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).
This does not look correct to me.  I'd definitely demand to see a separate patch with a clear test case justifying this.  


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


================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:779
         MII->getOpcode() == AArch64::DBG_VALUE ||
+        MII->getOpcode() == TargetOpcode::STATEPOINT ||
         MII->getOpcode() == TargetOpcode::PATCHPOINT ||
----------------
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)


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

https://reviews.llvm.org/D66012