[PATCH] D82917: [ThinLTO] parse flags and blockcount summaries

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Sorry looks like I missed this when you original sent it. The patches that added these summary types (D80403 <https://reviews.llvm.org/D80403> and D74420 <https://reviews.llvm.org/D74420>) did add the summary parsing support, but looks like they forgot to handle the case where we are trying to skip entries. Comments on that below.

In D82917#2124085 <https://reviews.llvm.org/D82917#2124085>, @nickdesaulniers wrote:

> Also, I didn't see these documented in https://llvm.org/docs/LangRef.html#thinlto-summary, is that expected?


It should be, I missed that in my review of those 2 patches. Thanks for the note, I can add that. Looks like that documentation is a bit out of date in other ways too. Will try to fix that up.



================
Comment at: llvm/lib/AsmParser/LLParser.cpp:791
   // Each module summary entry consists of a tag for the entry
   // type, followed by a colon, then the fields surrounded by nested sets of
   // parentheses. The "tag:" looks like a Label. Once parsing support is
----------------
This comment is a little stale, since the blockcount and flags entries don't follow this pattern, which is why you presumably needed to add calls to parse them below rather than going through the loop at the bottom that simply skips them. Can you update it?


================
Comment at: llvm/test/Assembler/thinlto-bad-summary1.ll:17
 
 ^0 = module: (path: "{{.*}}thinlto-bad-summary1.ll", hash: (0, 0, 0, 0, 0))
 ^1 = ()
----------------
Presumably this test would have failed if it had a blockcount or flags summary entry, since opt is currently invoking the skipping support. Can you add those summary types here (probably confirm that it is indeed failing at head and now able to parse those entries with your fix).


================
Comment at: llvm/test/Assembler/thinlto-summary.ll:66
 ^28 = typeid: (name: "_ZTS1E", summary: (typeTestRes: (kind: unsat, sizeM1BitWidth: 0)))
+^29 = flags: 8
+^30 = blockcount: 1888
----------------
Was this test actually failing without your fix? As noted earlier, parsing support was added for these fields, for when we are actually trying to build a module summary from the assembly, which is what this test case is doing. That being said, it is good to add this here for completeness (The flags summary type is being round trip tested by llvm/test/Assembler/summary-flags.ll, but I don't believe the blockcount is anywhere).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82917/new/

https://reviews.llvm.org/D82917