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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

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?

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.



================
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; }
 
----------------
Can this be split out and committed separately?


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


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


================
Comment at: tools/llvm-lto/llvm-lto.cpp:383
         ExitOnErr(errorOrToExpected(MemoryBuffer::getFileOrSTDIN(Filename)));
-    ExitOnErr(readModuleSummaryIndex(*MB, CombinedIndex, ++NextModuleId));
+    ExitOnErr(readModuleSummaryIndex(*MB, CombinedIndex, NextModuleId++));
   }
----------------
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.)


Repository:
  rL LLVM

https://reviews.llvm.org/D46699