[PATCH] D23885: [ThinLTO] add constArgumentsBitmask to caller summary

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

It seems like in order to make the bitmask useful (over a simple count of the number of constant args), we would at a minimum need to have info on the function summary as well (e.g. a bitmask of which formal parameters yield constant folding opportunities because they are compared against constants). And as Easwaran noted, in order to make good conclusions we may need to know what the constant values are (both passed at the callsite, and which are compared against in the callee function summary).

I think until there are some uses of this new info in the function importer and an exploration of the resulting gains (e.g. we start importing and therefore inlining additional profitable functions, or are able to use this to reduce the amount of importing without losing performance) we won't know which summary representation is useful. I'm a little concerned about bloating the summary index with an extra vbr per call edge without any data or specific use cases. I suggest adding the importer heuristic and collecting some data before pushing the index change upstream.

I have some additional comments below, but you probably don't want to address these until the above points are resolved.


================
Comment at: include/llvm/Bitcode/LLVMBitCodes.h:197
@@ -196,3 +196,3 @@
   // PERMODULE: [valueid, flags, instcount, numrefs, numrefs x valueid,
-  //             n x (valueid, callsitecount)]
+  //             n x (valueid, callsitecount, constArgumentsBitmask)]
   FS_PERMODULE = 1,
----------------
Naming convention for fields uses "_" not lower camel case.

================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:244
@@ +243,3 @@
+/// Specific info about the call site, like the info about the arguments.
+struct CallsiteInfo {
+  /// if n_th bit is set, it means that n_th argument is constant.
----------------
Since we are accumulating the info for all callsites, there isn't a reason to keep this in a separate structure from CalleeInfo. I.e. the profile counts are accumulated in CalleeInfo, even though that is essentially also callsite info.  If we decide to keep a separate edge per callsite eventually, the CalleeInfo struct will simply change to a CallsiteInfo struct and we would remove CallsiteCount. So better to have this info in CalleeInfo directly.

================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:125
@@ +124,3 @@
+              // setting it for every candidate which might result in
+              // nonsense data. It is worth investigating later.
+            }
----------------
It isn't clear why it would result in nonsense data - we are setting this based on arguments passed to the indirect call, so if argument N is constant, it presumably should be marked constant for each profiled target.

If there is a reason to not do it at this time, then IndirectCallEdges should go back to its previous type (no std::pair).

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:6276
@@ -6275,1 +6275,3 @@
+        CallsiteInfo Info;
+        Info.ConstArgumentsBitmask = Record[++I];
         uint64_t ProfileCount = HasProfile ? Record[++I] : 0;
----------------
Similar to Mehdi's comment below, this part needs to handle existing bitcode.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:3359
@@ -3356,3 +3358,3 @@
   Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 4));   // numrefs
   // numrefs x valueid, n x (valueid, callsitecount)
   Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Array));
----------------
Descriptions here and below record need to be updated.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:3444
@@ -3441,3 +3443,3 @@
   Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 4));   // numrefs
   // numrefs x valueid, n x (valueid, callsitecount)
   Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Array));
----------------
Descriptions here and below record need updating.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:3501
@@ -3500,3 +3502,2 @@
     assert(S);
-
     assert(hasValueId(I.first));
----------------
Remove newline change.


https://reviews.llvm.org/D23885