[ThinLTO] parse flags and blockcount summaries

[nickdesaulniers]

Summary

Forked from pr/46523, we were having a hard time running llvm-extract on
IR from a thinLTO build of the Linux kernel.

$ llvm-extract --func jeq_imm jit-42f488b63a04fdaa931315bdadecb6d23e20529a.ll
llvm-extract: jit-42f488b63a04fdaa931315bdadecb6d23e20529a.ll:47463:8:
error: Expected 'gv', 'module', or 'typeid' at the start of summary
entry
^209 = flags: 8

^

[nickdesaulniers]

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

[tejohnson]

Sorry looks like I missed this when you original sent it. The patches that added these summary types (D80403 and 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, @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.

llvm/lib/AsmParser/LLParser.cpp

790–791

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?

llvm/test/Assembler/thinlto-bad-summary1.ll

17

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).

llvm/test/Assembler/thinlto-summary.ll

66

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).

[nickdesaulniers]

• rebase, update comment

[nickdesaulniers]

No worries, thanks for taking a look.

llvm/test/Assembler/thinlto-bad-summary1.ll

17

If I add

+^2 = flags: 8
+^3 = blockcount: 1234

to HEAD (before this patch), the test will pass, because the parser stops after the first error. If I remove ^1 = (), the the test fails for ^2. I'm not sure I can add cases for flags or blockcount here, but I think the cases added to llvm/test/Assembler/thinlto-summary.ll provides coverage of this change to the parsing logic? WDYT?

llvm/test/Assembler/thinlto-summary.ll

66

No, it was not failing before my fix. These are the two test cases I added for coverage of my changes. These would be red before my changes to the parsing logic.

[tejohnson]

llvm/test/Assembler/thinlto-bad-summary1.ll

17

Hmm, right it will be difficult to distinguish a failure to parse the flags/blockcount vs "^1 = ()" since they would have been getting the same error message. I think what we would need to actually test the bug you fixed is another test like this one that invokes "opt" (so invokes SkipModuleSummaryEntry), but that doesn't have a bogus entry like ^1 = (). So without your fix to the parser it would presumably fail on the flags or blockcount summary, and with your fix it should not fail.

See my comment further below why I think the changes to thinlto-summary.ll are not actually testing your fix (although I think they are good to add for testing the existing parsing code).

llvm/test/Assembler/thinlto-summary.ll

66

Right, the test was passing before this patch, but I think it would still pass with your changes to the test (to add these two summary assembly entries) even without your changes to the parsing, because there is support for parsing these types of summary entries when we are building a summary index during parsing (which llvm-as does). Your change is to the code that skips the summary entries, which is invoked e.g. by 'opt' which AFAIK doesn't currently build a module summary from the assembly. I.e. the entries you added here would have been parsed successfully (I think) by where we were already calling ParseSummaryIndexFlags and ParseBlockCount.

[nickdesaulniers]

• add tests that are red before the patch, green after

[nickdesaulniers]

Ah, right, my mistake.

[tejohnson]

lgtm

llvm/lib/AsmParser/LLParser.cpp

if (FI != ForwardRefMDNodes.end()) {
     NumberedMetadata[MetadataID].reset(Init);
   }
 
   return false;
 }
 
 // Skips a single module summary entry.
 bool LLParser::SkipModuleSummaryEntry() {
   // 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
-  // in place we will look for the tokens corresponding to the expected tags.
+  // type, followed by a colon, then the fields which may be surrounded by
+  // nested sets of parentheses. The "tag:" looks like a Label. Once parsing
+  // support is in place we will look for the tokens corresponding to the
+  // expected tags.
   if (Lex.getKind() != lltok::kw_gv && Lex.getKind() != lltok::kw_module &&
-      Lex.getKind() != lltok::kw_typeid)
+      Lex.getKind() != lltok::kw_typeid && Lex.getKind() != lltok::kw_flags &&
+      Lex.getKind() != lltok::kw_blockcount)
     return TokError(
-        "Expected 'gv', 'module', or 'typeid' at the start of summary entry");
+        "Expected 'gv', 'module', 'typeid', 'flags' or 'blockcount' at the "
+        "start of summary entry");
+  if (Lex.getKind() == lltok::kw_flags)
+    return ParseSummaryIndexFlags();
+  if (Lex.getKind() == lltok::kw_blockcount)
+    return ParseBlockCount();
   Lex.Lex();
   if (ParseToken(lltok::colon, "expected ':' at start of summary entry") ||
       ParseToken(lltok::lparen, "expected '(' at start of summary entry"))
     return true;
   // Now walk through the parenthesized entry, until the number of open
   // parentheses goes back down to 0 (the first '(' was parsed above).
   unsigned NumOpenParen = 1;
   do {
bool LLParser::ParseSummaryIndexFlags() {
   assert(Lex.getKind() == lltok::kw_flags);
   Lex.Lex();
 
   if (ParseToken(lltok::colon, "expected ':' here"))
     return true;
   uint64_t Flags;
   if (ParseUInt64(Flags))
     return true;
+  if (Index)
     Index->setFlags(Flags);
   return false;
 }
 
 /// ParseBlockCount
 ///   ::= 'blockcount' ':' UInt64
 bool LLParser::ParseBlockCount() {
   assert(Lex.getKind() == lltok::kw_blockcount);
   Lex.Lex();
 
   if (ParseToken(lltok::colon, "expected ':' here"))
     return true;
   uint64_t BlockCount;
   if (ParseUInt64(BlockCount))
     return true;
+  if (Index)
     Index->setBlockCount(BlockCount);
   return false;
 }
 
 /// ParseGVEntry
 ///   ::= 'gv' ':' '(' ('name' ':' STRINGCONSTANT | 'guid' ':' UInt64)
 ///         [',' 'summaries' ':' Summary[',' Summary]* ]? ')'
 /// Summary ::= '(' (FunctionSummary | VariableSummary | AliasSummary) ')'
 bool LLParser::ParseGVEntry(unsigned ID) {

llvm/test/Assembler/thinlto-bad-summary1.ll

 ; Test that we get appropriate error when parsing summary with a missing
 ; summary type label.
 ; RUN: not opt %s 2>&1 | FileCheck %s
 
-; CHECK: error: Expected 'gv', 'module', or 'typeid' at the start of summary entry
+; CHECK: error: Expected 'gv', 'module', 'typeid', 'flags' or 'blockcount' at the start of summary entry
 
 ; ModuleID = 'thinlto-function-summary-callgraph.ll'
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 ; Function Attrs: nounwind uwtable
 define i32 @main() #0 {
 entry:
     ret i32 1
 }
 
 ^0 = module: (path: "{{.*}}thinlto-bad-summary1.ll", hash: (0, 0, 0, 0, 0))
 ^1 = ()

llvm/test/Assembler/thinlto-blockcount-summary.ll

+; Test that we can parse blockcount summaries.
+; RUN: opt %s -o /dev/null
+
+define void @main() {
+entry:
+    ret void
+}
+
+^0 = module: (path: "{{.*}}thinlto-bad-summary5.ll", hash: (0, 0, 0, 0, 0))
+^1 = blockcount: 1234

llvm/test/Assembler/thinlto-flags-summary.ll

+; Test that we can parse flag summaries.
+; RUN: opt %s -o /dev/null
+
+define void @main() {
+entry:
+    ret void
+}
+
+^0 = module: (path: "{{.*}}thinlto-flags-summary.ll", hash: (0, 0, 0, 0, 0))
+^1 = flags: 8

llvm/test/Assembler/thinlto-summary.ll

 ; Test TypeId with other optional fields (alignLog2/sizeM1/bitMask/inlineBits)
 ^25 = typeid: (name: "_ZTS1B", summary: (typeTestRes: (kind: inline, sizeM1BitWidth: 0, alignLog2: 1, sizeM1: 2, bitMask: 3, inlineBits: 4)))
 ; Test the AllOnes resolution, and all kinds of WholeProgramDevirtResolution
 ; types, including all optional resolution by argument kinds.
 ^26 = typeid: (name: "_ZTS1A", summary: (typeTestRes: (kind: allOnes, sizeM1BitWidth: 7), wpdResolutions: ((offset: 0, wpdRes: (kind: branchFunnel)), (offset: 8, wpdRes: (kind: singleImpl, singleImplName: "_ZN1A1nEi")), (offset: 16, wpdRes: (kind: indir, resByArg: (args: (1, 2), byArg: (kind: indir, byte: 2, bit: 3), args: (3), byArg: (kind: uniformRetVal, info: 1), args: (4), byArg: (kind: uniqueRetVal, info: 1), args: (5), byArg: (kind: virtualConstProp)))))))
 ; Test the other kinds of type test resoultions
 ^27 = typeid: (name: "_ZTS1D", summary: (typeTestRes: (kind: byteArray, sizeM1BitWidth: 0)))
 ^28 = typeid: (name: "_ZTS1E", summary: (typeTestRes: (kind: unsat, sizeM1BitWidth: 0)))
+^29 = flags: 8
+^30 = blockcount: 1888
 
 ; Make sure we get back from llvm-dis essentially what we put in via llvm-as.
 ; CHECK: ^0 = module: (path: "thinlto-summary1.o", hash: (1369602428, 2747878711, 259090915, 2507395659, 1141468049))
 ; CHECK: ^1 = module: (path: "thinlto-summary2.o", hash: (2998369023, 4283347029, 1195487472, 2757298015, 1852134156))
 ; CHECK: ^2 = gv: (guid: 1, summaries: (function: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), insts: 10, calls: ((callee: ^15, hotness: hot), (callee: ^17, hotness: cold), (callee: ^16, hotness: none)), refs: (^11, readonly ^13, writeonly ^14))))
 ; CHECK: ^3 = gv: (guid: 2, summaries: (function: (module: ^1, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), insts: 10, calls: ((callee: ^15)))))
 ; CHECK: ^4 = gv: (guid: 3, summaries: (function: (module: ^0, flags: (linkage: internal, notEligibleToImport: 0, live: 0, dsoLocal: 1, canAutoHide: 0), insts: 1)))
 ; CHECK: ^5 = gv: (guid: 4, summaries: (alias: (module: ^0, flags: (linkage: private, notEligibleToImport: 0, live: 0, dsoLocal: 1, canAutoHide: 0), aliasee: ^14)))

 ; CHECK: ^21 = gv: (guid: 20, summaries: (function: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), insts: 5, typeIdInfo: (typeCheckedLoadVCalls: (vFuncId: (^25, offset: 16))))))
 ; CHECK: ^22 = gv: (guid: 21, summaries: (function: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), insts: 15, typeIdInfo: (typeTestAssumeConstVCalls: ((vFuncId: (^27, offset: 16), args: (42)), (vFuncId: (^27, offset: 24)))))))
 ; CHECK: ^23 = gv: (guid: 22, summaries: (function: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), insts: 5, typeIdInfo: (typeCheckedLoadConstVCalls: ((vFuncId: (^28, offset: 16), args: (42)))))))
 ; CHECK: ^24 = typeid: (name: "_ZTS1C", summary: (typeTestRes: (kind: single, sizeM1BitWidth: 0))) ; guid = 1884921850105019584
 ; CHECK: ^25 = typeid: (name: "_ZTS1B", summary: (typeTestRes: (kind: inline, sizeM1BitWidth: 0, alignLog2: 1, sizeM1: 2, bitMask: 3, inlineBits: 4))) ; guid = 6203814149063363976
 ; CHECK: ^26 = typeid: (name: "_ZTS1A", summary: (typeTestRes: (kind: allOnes, sizeM1BitWidth: 7), wpdResolutions: ((offset: 0, wpdRes: (kind: branchFunnel)), (offset: 8, wpdRes: (kind: singleImpl, singleImplName: "_ZN1A1nEi")), (offset: 16, wpdRes: (kind: indir, resByArg: (args: (1, 2), byArg: (kind: indir, byte: 2, bit: 3), args: (3), byArg: (kind: uniformRetVal, info: 1), args: (4), byArg: (kind: uniqueRetVal, info: 1), args: (5), byArg: (kind: virtualConstProp))))))) ; guid = 7004155349499253778
 ; CHECK: ^27 = typeid: (name: "_ZTS1D", summary: (typeTestRes: (kind: byteArray, sizeM1BitWidth: 0))) ; guid = 9614786172484273522
 ; CHECK: ^28 = typeid: (name: "_ZTS1E", summary: (typeTestRes: (kind: unsat, sizeM1BitWidth: 0))) ; guid = 17437243864166745132
+; CHECK: ^29 = flags: 8
+; CHECK: ^30 = blockcount: 1888
 
 ; Make sure parsing of a non-summary entry containing a ":" does not fail
 ; after summary parsing, which handles colons differently.
 !0 = !DIFile(filename: "thinlto-summary.ll", directory: "", source: "")