[PATCH] D77830: [RFC] Generate more constant iVar Offsets via global LTO analysis

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith requested changes to this revision.
dexonsmith added a subscriber: tejohnson.
dexonsmith added a comment.
This revision now requires changes to proceed.

In D77830#1979631 <https://reviews.llvm.org/D77830#1979631>, @alexbdv wrote:

> I'll look into adding what needs to be added to have this also work for non-LTO. I was imagining It would be lot more integration.
>  Any other thoughts on this ?


Hi @alexbdv, thanks for working on this.

The patch as-is is fairly big and will be hard to review as a whole.  I have some high-level suggestions to help you split this up and add tests, which will make it easier to review and land.

I would suggest splitting the patch at least into the following pieces:

1. Add an IR analysis that figures out ivar constant offsets (unrelated to ThinLTO).
  - This is probably some combination/subset of the logic you have in `llvm/lib/Analysis/IVarOptAnalysis.cpp` and `llvm/lib/Analysis/ModuleSummaryAnalysis.cpp`, but you want to skip anything to do with ThinLTO for now.
  - Tests for analyses go in `llvm/test/Analysis`.  You can see examples of other Analysis tests there, but the idea is to hand-write/generate interesting IR and use `FileCheck` to confirm the analysis is correct.
2. Add an IR optimization pass that leverages the IR analysis pass.
  - This is probably fairly close to what you have in `llvm/lib/Transforms/IPO/IVarDirectAccess.cpp`.
  - Tests can go in `llvm/test/Transforms/`.  Check that IR gets transformed as you expect.
3. Add the IR optimization pass to the normal+FullLTO optimization pipelines.
  - Could be done later, not blocking anything later.
  - I would suggest adding a FullLTO test.
4. Store necessary bits of analysis in the ThinLTO summary.
  - You might want to split out bitcode reading/writing separately from this, or maybe it makes sense to keep them together.
  - This can be tested in isolation, that analysis information gets correctly merged, etc.
5. Upgrade the IR analysis pass to leverage information from a ThinLTO summary.
  - I believe you can test the analysis pass here.
  - I believe you can test that the optimization will do the right thing here as well.
6. Add the IR optimization pass to the ThinLTO optimization pipeline.
  - Not sure if we usually test which passes are in the pipeline... you can check for other examples.

Besides making it easier to review, splitting the patch up will allow you to land incremental patches and test separable logic in isolation.  Note that it's also quite valuable to separate out patches that add passes to standard pipelines so they can be reverted in isolation if there are regressions (without backing out all the other logic).

@tejohnson, do you have other suggestions on how to split this up?  Or more specific advice on testing the ThinLTO bits?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77830