[PATCH] D46699: [ThinLTO] Print module summary index to assembly

Wed May 23 09:31:52 PDT 2018

dexonsmith added a comment.

The LangRef looks great, thanks for adding it.  I have a few nitpicks and a follow-up on the `SkipLineComment` discussion.  I made it most of the way through the AssemblyWriter (and any comments/questions there might apply more generally to the rest)... you may as well respond to those first, and then I can review the rest.

================
Comment at: docs/LangRef.rst:5707-5708
+
+ThinLTO Summary
+===============
+
----------------
Is this a good place to document the behaviour around when the ThinLTO summary does and does not get dropped?

================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:434-435
+  struct TypeIdInfo {
+    /// List of type identifiers used by this function in llvm.type.test
+    /// intrinsics other than by an llvm.assume intrinsic, represented as GUIDs.
+    std::vector<GlobalValue::GUID> TypeTests;
----------------
This comment isn't clear to me.  The current text seems to suggest that the `llvm.assume` intrinsic is a type of `llvm.type.test` intrinsic.  Assuming that's unintentional, I wonder if one of these would be accurate:
- "List of type indentifiers used by this function in llvm.type.test intrinsics not referenced by an llvm.assume intrinsic, ..."
- "List of type indentifiers used by this function in llvm.type.test intrinsics referenced by something other than an llvm.assume intrinsic, ..."

(I just noticed this is an old comment that has just been moved, so feel free to clean up separately.)

================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:871-873
+  ValueInfo getValueInfo(const GlobalValueSummaryMapTy::value_type *R) const {
+    return ValueInfo(IsAnalysis, R);
+  }
----------------
Is it necessary to allow `nullptr` here?  If not, can we take `GlobalValueSummaryMapTy::value_type` by reference?

================
Comment at: lib/AsmParser/LLLexer.cpp:433-436
+    uint64_t Val = atoull(TokStart + 1, CurPtr);
+    if ((unsigned)Val != Val)
+      Error("invalid value number (too large)!");
+    UIntVal = unsigned(Val);
----------------
It looks like this is copied exactly from elsewhere.  Can/should we factor this out into `LexUIntID` (or something)?  (Maybe even taking the token to return on success?)

================
Comment at: lib/AsmParser/LLLexer.h:68-69
     void Warning(const Twine &Msg) const { return Warning(getLoc(), Msg); }
+    // FIXME: Make private again when summary parsing support is complete.
+    void SkipLineComment();

----------------
tejohnson wrote:
> dexonsmith wrote:
> > tejohnson wrote:
> > > dexonsmith wrote:
> > > > Rather than making this (temporarily) public, it might be nicer to add a function like "LexModuleSummaryIndex" that calls `SkipLineComment`.  Then it'll be less likely that `SkipLineComment` grows non-`private` users.
> > > I realized when working on the parsing that it is going to be useful to keep this functionality public, because for parsing clients that aren't interested in the index, for which we will pass in a null Index pointer, we want to continue to skip the index lines. Similarly, for clients that aren't interested in the IR (e.g. the Thin Link, which when reading a bitcode file skips past all of the IR records), we will pass the parser a null Module pointer and want to skip all the non-summary IR lines. I can add a new public function to call this, not sure of the best name, or just rename SkipLineComment to SkipLine.
> > Or perhaps a more honest name like `SkipModuleSummaryIndex`.
> I can add that for this patch. But that isn't a good name for the other use case I am adding to my parsing patch, of skipping the IR when we only care about reading the summary index. Should we have two interfaces (e.g. SkipModuleSummaryIndex and SkipModuleIR) or a single generic skip interface?
Interesting.  Are you sure `SkipModuleIR` will do what you want if it just skips to the end of a line?  IIRC, IR can be split over multiple lines (e.g., metadata).

Assuming it works, I might slightly prefer the explicit names for each use case, but I'm not sure I have a good reason to...

If you do keep it generic, I think `SkipLine` (as you suggested elsewhere) is a better name.

================
Comment at: lib/IR/AsmWriter.cpp:765
+  /// GUID map iterators.
+  using GUID_iterator = DenseMap<GlobalValue::GUID, unsigned>::iterator;
+
----------------
This is a weird mix of naming styles.  I think it should either be `GUIDIterator` (which matches our usual naming style) or `guid_iterator` (which matches `as_iterator` above).

================
Comment at: lib/IR/AsmWriter.cpp:767-768
+
+  GUID_iterator GUID_begin() { return GUIDMap.begin(); }
+  GUID_iterator GUID_end() { return GUIDMap.end(); }
+
----------------
I don't see these being used anywhere.

================
Comment at: lib/IR/AsmWriter.cpp:907

+inline void SlotTracker::initializeIndex() {
+  if (TheIndex) {
----------------
Does marking this as `inline` really gain anything here (besides a weak symbol)?  I doubt our inliner really needs a hint here.  But I could be wrong...

================
Comment at: lib/IR/AsmWriter.cpp:908
+inline void SlotTracker::initializeIndex() {
+  if (TheIndex) {
+    processIndex();
----------------
Invert condition and return early to reduce nesting?

================
Comment at: lib/IR/AsmWriter.cpp:1011
+  // assigned consecutively. Start numbering the GUIDs after the module ids.
+  GUIDNext = TheIndex->modulePaths().size();
+
----------------
Is it useful to `assert(TheIndex)`?

================
Comment at: lib/IR/AsmWriter.cpp:1176-1177
+void SlotTracker::CreateGUIDSlot(GlobalValue::GUID GUID) {
+  unsigned DestSlot = GUIDNext++;
+  GUIDMap[GUID] = DestSlot;
+}
----------------
The `DestSlot` local seems unnecessary here...

================
Comment at: lib/IR/AsmWriter.cpp:2261
   const Module *TheModule;
+  const ModuleSummaryIndex *TheIndex;
   std::unique_ptr<SlotTracker> SlotTrackerStorage;
----------------
`= nullptr`?

================
Comment at: lib/IR/AsmWriter.cpp:2370-2371
+                               const ModuleSummaryIndex *Index, bool IsForDebug)
+    : Out(o), TheModule(nullptr), TheIndex(Index), Machine(Mac),
+      TypePrinter(nullptr), AnnotationWriter(nullptr), IsForDebug(IsForDebug),
+      ShouldPreserveUseListOrder(false) {}
----------------
I'd prefer these `nullptr` assignments to be at the declaration in the class.

================
Comment at: lib/IR/AsmWriter.cpp:2581
+
+  // First print module path entries, using the module id as the slot number. To
+  // print in order, add paths to a vector indexed by module id.
----------------
I feel like "First" is liable to bitrot.

================
Comment at: lib/IR/AsmWriter.cpp:2609-2610
+
+  // FIXME: Change AliasSummary to hold a ValueInfo instead of summary pointer
+  // for aliasee (then update BitcodeWriter.cpp and remove get/setAliaseeGUID).
+  for (auto &GlobalList : *TheIndex) {
----------------
- Period at the end of the sentence.
- Did you intend to leave this FIXME until later?

================
Comment at: lib/IR/AsmWriter.cpp:2617
+
+  // Next print the global value summary entries.
+  for (auto &GlobalList : *TheIndex) {
----------------
I think "Next" is unnecessary here.

================
Comment at: lib/IR/AsmWriter.cpp:2624
+
+  // Last print the TypeIdMap entries.
+  for (auto &TId : TheIndex->typeIds()) {
----------------
I think "Last" is liable to bitrot.

================
Comment at: lib/IR/AsmWriter.cpp:2684
+  // of absolute symbols to store constants. Print only if non-zero.
+  if (TTRes.AlignLog2 || TTRes.SizeM1 || TTRes.BitMask || TTRes.InlineBits) {
+    Out << ", alignLog2: " << TTRes.AlignLog2;
----------------
Why print all or none?  (Would it be better to check each for non-zero?)

================
Comment at: lib/IR/AsmWriter.cpp:2698
+  printTypeTestResolution(TIS.TTRes);
+  if (TIS.WPDRes.size()) {
+    Out << ", wpdResolutions: (";
----------------
Is there an `empty` method to use instead?

Repository:
  rL LLVM

https://reviews.llvm.org/D46699