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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D46699#1100613, @dexonsmith wrote:

> I haven't had time to give this a full review, but skimming this I noticed that there's no update to llvm/docs/LangRef.rst.  Can you document the syntax and semantics there?


Thanks for taking a look! Good point, I will work on the LangRef changes and add to this patch.

> I also wonder if it would be cleaner to add support for different constructs one-at-a-time, adding printing/parsing/double-round-trip-testing/LangRef incrementally for each construct.  But I'm not totally opposed to doing the printing support first.

It's a lot easier to do it all at once. Building a partial index on the parsing side has its own issues.



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:315
   /// Get the flags for this GlobalValue (see \p struct GVFlags).
-  GVFlags flags() { return Flags; }
+  GVFlags flags() const { return Flags; }
 
----------------
dexonsmith wrote:
> Can this be split out and committed separately?
Yes, will do.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:519
   /// Get function attribute flags.
-  FFlags &fflags() { return FunFlags; }
+  FFlags fflags() const { return FunFlags; }
 
----------------
dexonsmith wrote:
> Can this be split out and committed separately?
Yes, will do.


================
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();
 
----------------
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.


================
Comment at: tools/llvm-lto/llvm-lto.cpp:383
         ExitOnErr(errorOrToExpected(MemoryBuffer::getFileOrSTDIN(Filename)));
-    ExitOnErr(readModuleSummaryIndex(*MB, CombinedIndex, ++NextModuleId));
+    ExitOnErr(readModuleSummaryIndex(*MB, CombinedIndex, NextModuleId++));
   }
----------------
dexonsmith wrote:
> It's not obvious to me if this is fixing a pre-existing bug or you need to change this to count from 0 instead of from 1.  (If the former, you may as well separate this out into a prep commit.)
Yeah it's a pre-existing inconsistency with the module numbering done by both the new and old LTO APIs used by linkers, which both start at 0. I will commit this separately.


Repository:
  rL LLVM

https://reviews.llvm.org/D46699