[PATCH] D89249: [SimplifyLibCalls] Preserve tail call indicator
Nick Desaulniers via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 13 10:57:43 PDT 2021
nickdesaulniers added inline comments.
================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1735-1736
++NumSimplified;
- return CI->use_empty() ? CI : replaceInstUsesWith(*CI, With);
+ if (CI->use_empty())
+ return CI;
+
----------------
If we're not replacing `CI` with the simplified version `With`, why do we even bother to compute `With`? This guard looks to me like it should be hoisted above the call to `LibCallSimplifier::optimizeCall`. Otherwise `NumSimplified` seems like a useless statistic. Who cares that I made a simplification //if I didn't even **use** it?//
I'm also curious why `CI` is returned rather than `nullptr`?
================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1738-1739
+
+ if (auto *NewCall = dyn_cast<CallInst>(With))
+ NewCall->setTailCallKind(CI->getTailCallKind());
+ return replaceInstUsesWith(*CI, With);
----------------
As discusses in D107872, this should probably be:
```
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index aa806325bcf5..f749aca6083f 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -2433,13 +2433,13 @@ Instruction *InstCombinerImpl::tryOptimizeCall(CallInst *CI) {
if (auto *NewCI = dyn_cast<CallInst>(With))
NewCI->setTailCallKind(CI->getTailCallKind());
+ else if (CI->isMustTailCall())
+ return nullptr;
+ ++NumSimplified;
```
The the update to `NumSimplified` stat should come after the potential `return nullptr`.
================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1741
+ return replaceInstUsesWith(*CI, With);
}
----------------
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89249/new/
https://reviews.llvm.org/D89249
More information about the llvm-commits
mailing list