[PATCH] Add support for symbolic large constant entries inside stackmaps

[Philip Reames]

Comments inline, but you are still missing the most important part of the patch.  Where is the documentation which describes the output format?  I have inferred some of it from the code, but it will save you time if we can settle on the output before hashing through every bit of code.  We may need to change the code based on the result of the format discussion.

One question to consider: As a consumer of the generated stackmap, how do I tell a given entry is a actual constant vs a symbol that needs resolved?  Maybe I'm missing the obvious, but I didn't see this in the code.  Or are you assuming that the consumer is only parsing the finalized/full relocated version of the binary section?


REPOSITORY
  rL LLVM

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6920
@@ +6919,3 @@
+    } else if (auto *GA = dyn_cast<GlobalAddressSDNode>(OpVal)) {
+      if (GA->getValueType(0) != MVT::i64)
+        Ops.push_back(OpVal);
----------------
I don't understand this check.  What case is this for?  Should this possibly be an assert?

Or should you possibly be using the getValueType in place of the MVT::i64 as an argument to getTargetGlobalAddress?

================
Comment at: lib/CodeGen/StackMaps.cpp:309
@@ -298,3 +308,3 @@
     // -1 is directly encoded as .long 0xFFFFFFFF with no constant pool.
-    if (I->LocType == Location::Constant && !isInt<32>(I->Offset)) {
+    if (I->LocType == Location::Constant && !isInt<32>(I->Offset) && !I->Sym) {
       I->LocType = Location::ConstantIndex;
----------------
The need for this check makes me thing that you should introduce a new Location type. 

================
Comment at: lib/CodeGen/StackMaps.cpp:329
@@ +328,3 @@
+       ++I) {
+    if (I->LocType == Location::Constant && I->Sym) {
+      I->LocType = Location::ConstantIndex;
----------------
I really think you want a Location::GlobalValue or something similar.  

================
Comment at: lib/CodeGen/StackMaps.cpp:333
@@ +332,3 @@
+      // The symbolic entries will be emitted after the ConstPool entries.
+      I->Offset = ConstPool.size() + Result.first - ConstSymPool.begin();
+    }
----------------
This seems unnecessarily complicated.  Just emit the offset from the beginning of the global value section.  (This comment is dependent on the one below w.r.t. reuse of constant section.)

================
Comment at: lib/CodeGen/StackMaps.cpp:420
@@ +419,3 @@
+  auto NumConst = ConstPool.size() + ConstSymPool.size();
+  DEBUG(dbgs() << WSMP << "#constants = " << NumConst << '\n');
+  OS.EmitIntValue(NumConst, 4);
----------------
Mixing global values with constants doesn't seem like the right approach here.  Why not introduce new section specifically for global values?

(I'm open to being convinced this is the right answer, but you need to make the argument.)

http://reviews.llvm.org/D9176

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/