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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

No worries, thanks for taking a look.



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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82917