[PATCH] D13110: New StackMap format

[Philip Reames via llvm-commits]

reames added a subscriber: reames.
reames added a comment.

I've included some larger comments but have not bothered to do a full review.  This change will need a lot of work before being ready to land.  I strongly suggest separating out the refactorings from this patch to make it easier to review.


================
Comment at: include/llvm/CodeGen/StackMaps.h:130
@@ -129,1 +129,3 @@
 
+/// StackMaps Version 2
+class StackMapsV2 {
----------------
There is no reason to duplicate the entire class just to version the serialization format.  Please fix this.

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:968
@@ -967,3 +967,3 @@
   if (!MMI->getLandingPads().empty() || MMI->hasDebugInfo() ||
-      MAI->hasDotTypeDotSizeDirective()) {
+      MAI->hasDotTypeDotSizeDirective() || SM.hasFrameMap()) {
     // Create a symbol for the end of function.
----------------
I'm a bit confused, why are these now conditional on whether there's a stackmap?

(p.s. terminology wise we've been using StackMap.  Please keep doing so.  Changing it everywhere might be fine, changing it piecemeal is not.)

================
Comment at: lib/CodeGen/StackMaps.cpp:371
@@ -344,1 +370,3 @@
 void StackMaps::recordStackMap(const MachineInstr &MI) {
+  if (StackMapVersion == 2) {
+    SM2.recordStackMap(MI);
----------------
This hook and forward patterns is unacceptably complex.  Most of the code is the same for each version and can be shared.  

================
Comment at: lib/Target/AArch64/AArch64AsmPrinter.cpp:51
@@ -50,3 +50,2 @@
   AArch64MCInstLower MCInstLowering;
-  StackMaps SM;
 
----------------
This appears to be a refactoring.  Please separate into it's own change.  

================
Comment at: lib/Target/X86/X86MCInstLower.cpp:877
@@ -873,1 +876,3 @@
+    CallLabel = OutContext.createTempSymbol();
+    OutStreamer->EmitLabel(CallLabel);
     OutStreamer->EmitInstruction(CallInst, getSubtargetInfo());
----------------
What's this needed for?  We already have a label immediately after the call inst.

Also, ordering of the new lines w.r.t. old is confusing.

================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:220
@@ -219,1 +219,3 @@
 
+  cl::opt<int> StackMapVersion(
+      "stackmap-version", cl::init(1),
----------------
The version is part of the file format.  This is unnecessary. 


http://reviews.llvm.org/D13110