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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/test/Assembler/thinlto-bad-summary1.ll:17
 
 ^0 = module: (path: "{{.*}}thinlto-bad-summary1.ll", hash: (0, 0, 0, 0, 0))
 ^1 = ()
----------------
nickdesaulniers wrote:
> tejohnson wrote:
> > 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).
> 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?
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).


================
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
----------------
nickdesaulniers wrote:
> tejohnson wrote:
> > 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).
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82917