[PATCH] D109294: [Inline] Remove dead users before checking if function has one use (PR51667)

Thu Sep 9 18:05:21 PDT 2021

mtrofin added inline comments.

================
Comment at: llvm/lib/Analysis/InlineCost.cpp:1862

+  F.removeDeadConstantUsers();
   bool OnlyOneCallAndLocalLinkage =
----------------
aeubanks wrote:
> erikdesjardins wrote:
> > mtrofin wrote:
> > > aeubanks wrote:
> > > > mtrofin wrote:
> > > > > nikic wrote:
> > > > > > aeubanks wrote:
> > > > > > > mtrofin wrote:
> > > > > > > > From a separation of concerns perspective, I think this should rather happen after a successful inline, rather than when computing cost.
> > > > > > > +1, an analysis shouldn't be changing the IR
> > > > > > This already happens after a successful inline. The problem is that we also need it to happen before the hasOneUse() check below. The operation could be move to the point where the cost model is queried though.
> > > > > Hmm... IIUC, right now, after the first time a callee is inlined, removeDeadConstantUsers() is called, and next time its cost is computed it'll be accurate. The problem is the first time, correct? Does this feel like a normalization pass is missing, i.e. before inlining, we go over all functions in a module and removeDeadConstantUsers()?
> > > > Could you move this to before `Advisor.getAdvice()` in `InlinerPass::run()`?
> > > wouldn't that still do it more than the necessary number of times? 
> > Moved to before `Advisor.getAdvice()`
> yeah we're calling removeDeadUsersOfConstant more than necessary, but I don't think it should matter that much. I don't think it's worth all the extra code and extra pass to optimize this
Looking more at it - it modifies IR though, right? I'm a bit troubled that it modifies IR and there's no trigger (unless I'm missing something) invalidating analyses. Do we handle the Uses/Users relations differently for some reason?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109294