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

[Nick Desaulniers via Phabricator via llvm-commits]

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