Page MenuHomePhabricator

[ThinLTO] parse flags and blockcount summaries
ClosedPublic

Authored by nickdesaulniers on Jun 30 2020, 3:05 PM.

Details

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

^

Diff Detail

Event Timeline

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

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.

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 marked 3 inline comments as done.
  • rebase, update comment

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 added inline comments.Fri, Jul 17, 2:14 PM
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.

  • add tests that are red before the patch, green after
nickdesaulniers marked 4 inline comments as done.Fri, Jul 17, 4:23 PM

Ah, right, my mistake.

This revision is now accepted and ready to land.Fri, Jul 17, 4:33 PM
This revision was automatically updated to reflect the committed changes.