[PATCH] D94334: [InlineAdvisor] Allow replay of inline decisions for the CGSCC inliner from optimization remarks

[Di Mo via Phabricator via llvm-commits]

modimo added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:669
+
+  if (!CGSCCInlineReplayFile.empty()) {
+    if (!ReplayAdvisor)
----------------
wenlei wrote:
> Plug in replay inline advisor here isn't extensible. In the future we want to be able to use inline replay only for a specific function, or enforce/prevent certain inlining at particular callsite, and fall back to  regular advisor for the rest (see comments in D83743). 
> 
> That means we would need to be able to fall back from replay advisor to default advisor (or whatever main advisor being used) when replay advisor doesn't have info. For that cascaded model, we would need inline advise to have something like `hasInlineRecommendation` in addition to `isInliningRecommended`. We should probably still record inlining on each advice, but don't want to emit duplicated remarks from each advice. These changes can come later, but current change better offer that flexibility - we don't to stick to replay advisor for the entire module inliner pass.  
I think your suggested change is to initialize both Advisors and allow fallback if we defer on one. With how the current scheme is setup up though advisors are single entities and we only ask it once:
`auto Advice = Advisor.getAdvice(*CB);`
A proven approach to doing something like this is with alias analysis where we query AA in a specific order until we hit a real recommendation. That would cause a rehaul of this function rather than a small tweak so I don't see a good intermediate step to take for this patch. Let me know if you think there's something to do here now for it.


================
Comment at: llvm/test/Transforms/Inline/cgscc-inline-replay.ll:4
+;; Check replay inline decisions
+; RUN: opt < %s -passes=inline -cgscc-inline-replay=%S/Inputs/cgscc-inline-replay.txt -pass-remarks=inline -S 2>&1 | FileCheck -check-prefix=REPLAY %s
+
----------------
wenlei wrote:
> Better verify inline decision without replay first, to make sure the replay has visible impact on inlining. See DEFAULT and REPLAY check for SampleProfile/inline-replay.ll.
Makes sense, added.


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

https://reviews.llvm.org/D94334