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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D77830#1981563 <https://reviews.llvm.org/D77830#1981563>, @dexonsmith wrote:

> 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?


Thanks for the heads up, I missed this patch initially. I only very quickly skimmed the review thread just now, but if this is an optimization that does not require whole program analysis I agree with others that it makes sense to implement it as an IR based analysis/optimization pass first, and extend to ThinLTO as a follow on. Doing it on IR will also make it trivial to support for regular LTO, where the pass just needs to be added to that pipeline. For the ThinLTO bits, the suggested breakdown looks pretty good. You could send just the ModuleSummaryIndex changes with a textual summary based test first (checking the summary output of llvm-dis), then follow up with the extension to look at the ThinLTO summary instead of IR for the analysis (I'm more ambivalent about whether step 5 and 6 need to be separated or not, but incremental patches are certainly easier to digest and review). For testing, use a tool like llvm-lto2 (see existing tests under llvm/test/LTO/). Feel free to add me as the reviewer for the ThinLTO specific changes, I'm happy to review that. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77830