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

Thu May 24 10:30:52 PDT 2018

tejohnson marked 17 inline comments as done.
tejohnson added a comment.

Thanks for the comments! I think I have addressed them all.

================
Comment at: docs/LangRef.rst:5707-5708
+
+ThinLTO Summary
+===============
+
----------------
dexonsmith wrote:
> Is this a good place to document the behaviour around when the ThinLTO summary does and does not get dropped?
Added (with a note about current temporary behavior).

================
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;
----------------
dexonsmith wrote:
> 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.)
Looking at the code, your second alternative appears to be correct. This is pcc's code and comment, I will send a change to him separately - assuming that goes in first I will merge with this patch.

================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:871-873
+  ValueInfo getValueInfo(const GlobalValueSummaryMapTy::value_type *R) const {
+    return ValueInfo(IsAnalysis, R);
+  }
----------------
dexonsmith wrote:
> Is it necessary to allow `nullptr` here?  If not, can we take `GlobalValueSummaryMapTy::value_type` by reference?
In my single callsite it will never be nullptr. In general, a ValueInfo can have a null pointer though. But given the documented use case (when iterating the index), it shouldn't be null, so I will change to a reference.

================
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:
> 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.
Hmm, it looks like metadata (at least) can be split across multiple lines, so my parsing patch is going to have to do more work to properly skip everything before the summary. I can probably do something simple there like skipping every line that doesn't start with "^".  For now I am assuming that the summary entries will be on a single line, so using SkipLineComment should work. I did rename SkipLineComment to SkipLine, which is actually what it is doing. I left it public but removed the FIXME, and added a LLParser method to skip module summary entries by invoking it.

================
Comment at: lib/IR/AsmWriter.cpp:765
+  /// GUID map iterators.
+  using GUID_iterator = DenseMap<GlobalValue::GUID, unsigned>::iterator;
+
----------------
dexonsmith wrote:
> 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).
Changed to guid_iterator to be consistent with other iterators.

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

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

+inline void SlotTracker::initializeIndex() {
+  if (TheIndex) {
----------------
dexonsmith wrote:
> 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...
Probably not, I just inherited this because I copied SlotTracker::initialize. Removed.

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

================
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) {
----------------
dexonsmith wrote:
> - Period at the end of the sentence.
> - Did you intend to leave this FIXME until later?
Yes, for now. I'd like to revisit this soon, but wanted to focus on getting the parsing side support done first.
Can you clarify your concern about the period at the end of the sentence? There's already one there, which I assume is correct.

================
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;
----------------
dexonsmith wrote:
> Why print all or none?  (Would it be better to check each for non-zero?)
In fact, my parsing WIP patch will handle any subset of these being specified. Fixed here.

================
Comment at: lib/IR/AsmWriter.cpp:2698
+  printTypeTestResolution(TIS.TTRes);
+  if (TIS.WPDRes.size()) {
+    Out << ", wpdResolutions: (";
----------------
dexonsmith wrote:
> Is there an `empty` method to use instead?
Yep, fixed here and elsewhere in patch.

Repository:
  rL LLVM

https://reviews.llvm.org/D46699