[PATCH] D89249: [SimplifyLibCalls] Preserve tail call indicator

Mon Oct 18 11:05:17 PDT 2021

efriedma added inline comments.

================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1741
+    return replaceInstUsesWith(*CI, With);
   }

----------------
nickdesaulniers wrote:
> efriedma wrote:
> > nickdesaulniers wrote:
> > > efriedma wrote:
> > > > xbolva00 wrote:
> > > > > jdoerfert wrote:
> > > > > > Can `omptimizeCall` ever break the tail requirements, e.g., create an alloca and pass it?
> > > > > I dont think so. @efriedma , what do you think?
> > > > > 
> > > > > 
> > > > I don't like this approach. I mean, I get the motivation, but I'm not sure this is the right way to address it.  Three issues:
> > > > 
> > > > 1. Even if we don't break the "tail" requirements right now, it's likely if someone did introduce a way, they wouldn't notice this code (which isn't even in the same file).
> > > > 2. A lot of combines create calls, but return some other value.
> > > > 3. For the floating-point routines, we probably want to add "tail" unconditionally.
> > > After spending lots of time on this in D107872, this does look like the best approach to me.
> > > 
> > > Regarding @efriedma points above, corresponding responses:
> > > 1. This comment doesn't make sense to me, can you please elaborate? Are you referring to `musttail`? Because `tail` can appear anywhere; it's just a hint, and is kind of pointless/redundant IMO (see below).
> > > 2. Right, and this is guarded against as written with the `dyn_cast` check. It just needs to be refined to not do the replacement if the replaced call is `musttail` and the new `Value` isn't also a `CallInst`.  I think this is was @jdoerfert was getting at above.
> > > 3. That should really be a separate CL; it's orthogonal to not dropping the tail call kind.  I'm not sure I even agree with that; honestly I don't get the point of `tail`.  We have "definitely" (`musttail`), "definitely not" (`notail`), and two "maybes" (`tail`, <no tail call kind>).  I honestly believe that we could get rid of `tail` from the IR outright, and just always attempt a tail call if a `CallInst` were the penultimate `Instruction`.  And if it's just an easily ignored hint, why not sprinkle it just everywhere? Why just the fp routines?
> > The plain "tail" marker is a marker for optimization: it means the called function doesn't refer to the caller's stack frame (allocas etc.).  That's complicated to deduce.  And sometimes the frontend has extra information which isn't represented otherwise. Also, the information is actually used in a few places outside of codegen (see BasicAAResult::getModRefInfo).  So it's not as simple as just "make the backend compute it".
> > 
> > It was arguably a mistake that "notail" is stored in the same enum as plain "tail". In theory, a "tail notail" call is reasonable.  It would mean that the callee doesn't refer to the caller's stack frame, but we're forbidden from tail calling for some other reason.  I think it ended up the way it did because musttail implies tail, and nobody reconsidered when notail was added.
> > 
> > -----
> > 
> > Two specific cases where I'm concerned this patch breaks down:
> > 
> > 1. The code in LibCallSimplifier creates an alloca for some reason. Then the created call might would not be legal to mark tail, even if the original call is marked tail.  I'm pretty sure it doesn't do that currently, but there isn't any obvious reason that won't ever happen.
> > 2. LibCallSimplifier returns one of the arguments to the original call.  In general, this isn't a call... but it can be, and if it is, the call you're modifying is completely unrelated to the call that was marked "tail".
> Thanks for the additional info/feedback.
> 
> Then the alternative is to sprinkle these "copy the tail call kind unless `musttail`" checks in basically every transform, carefully, on a case-by-case basis.  ie. as is done in https://reviews.llvm.org/D107872.  Are you ok with that approach then? Based on what you've written above, I think the asserts I added to llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp are ok to stay?
> 
> Can you please comment on the alternate approach taken in https://reviews.llvm.org/D107872?
The approach in D107872 seems generally fine; I'll comment there.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89249