[PATCH] D145209: [DAE] Don't DAE if we musttail call a "live" (non-DAE-able) function

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp:526
+      // In addition, if the called function is virtual, we can't change
+      // the return type.
+      if (!TC->getCalledFunction())
----------------
mtrofin wrote:
> efriedma wrote:
> > mtrofin wrote:
> > > efriedma wrote:
> > > > mtrofin wrote:
> > > > > nikic wrote:
> > > > > > mtrofin wrote:
> > > > > > > efriedma wrote:
> > > > > > > > Please explain why this matters.  Intuitively, whether a function is virtual shouldn't affect liveness... do we handle the non-virtual case somewhere else?
> > > > > > > Not sure I understand. It's about virtual musttail calls, not just virtual. "Liveness" in this pass means "it can't change".
> > > > > > > 
> > > > > > > Without the patch, the test fails in the validator, because the module is transformed to:
> > > > > > > 
> > > > > > > ```
> > > > > > > define internal void @test(ptr %fptr, i32 %a, i32 %b) {
> > > > > > >   %r = musttail call i32 %fptr(ptr %fptr, i32 %a, i32 0)
> > > > > > >   ret void
> > > > > > > }
> > > > > > > 
> > > > > > > ```
> > > > > > > 
> > > > > > > The error is what the test checks isn't present: `cannot guarantee tail call due to mismatched parameter counts`
> > > > > > > 
> > > > > > > The patch is actually incomplete - about to update it. This marking needs to be also propagated to users of the function.
> > > > > > > 
> > > > > > I believe Eli's point is that this is also true for non-virtual functions. For example, this example without virtual calls fails in the same way:
> > > > > > ```
> > > > > > define internal i32 @test() {
> > > > > >   %r = musttail call i32 @foo()
> > > > > >   ret i32 %r
> > > > > > }
> > > > > > 
> > > > > > declare i32 @foo()
> > > > > > ```
> > > > > Oh, I see. Let me update then.
> > > > Please add a comment explaining why direct calls and indirect calls are handled differently.  (Is it related to the usage of getCalledFunction() in surveyUse?)
> > > Ah, it's not "direct" vs "indirect", rather, it's "can the callee be statically known and is it defined in this module". If it is, then we know that whatever transformations we plan on making at the callsite will also be reflected in the body of that callee. But if it's not, we don't know if that could happen. 
> > > 
> > > Declarations ended up being marked as "live" just below. So we'd pick up that in `propagateVirtMustcallLiveness`.
> > > 
> > > All that being said, I think handling both callee cases here is more clear (+clarifying comment)
> > > 
> > > WDYT?
> > The concept makes sense to me... I'd prefer some more obvious way to quantify "calls to functions we can't analyze" to make it more obvious this is the inverse of the calls we do analyze.
> Would a `isCalleeAnalyzable` member that does the `!TC->getCalledFunction() || TC->getCalledFunction()->isDeclaration()` address that?
That's reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145209