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

[Loïc Ottet via Phabricator via llvm-commits]

loicottet marked an inline comment as done.
loicottet added inline comments.


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


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

https://reviews.llvm.org/D66012