[PATCH] D53892: [CodeGen] Support custom format of stack maps

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

A few drive by comments for the moment.



================
Comment at: include/llvm/CodeGen/GCMetadataPrinter.h:68
+  /// returns false and the default format will be used.
+  virtual bool emitStackMaps(StackMaps &SM, AsmPrinter &AP) { return false; }
 };
----------------
This new call is arguably duplicating the existing finishModule callback.  It'd be nice to find a way to merge the two honestly.  


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3005-3007
+      if (MP->emitStackMaps(SM, *this))
+        // We've emitted a custom format, done.
+        return;
----------------
cherry wrote:
> apilipenko wrote:
> > I'm not sure that early termination is the right thing to do. There might be more than one GC strategy in use in the current module, and all of them might want to emit stack maps.
> In theory there could be more than one GC strategies, although I couldn't see how it can be used.
> 
> What about changing it to running all the GCMetadataPrinters, then return if any of the emitStackMaps returns true?
> 
The structure of the existing code is that a single model can contain functions compiled with two different gc strategies.  I agree it's a bit odd, but it should continue to work.

I'd also like to move towards being able to emit *multiple* formats for a single GC.  (Primarily for testing purposes.)  That can be out of scope for your change if you desire.  


https://reviews.llvm.org/D53892