[PATCH] D107872: [SimplifyLibCalls] propagate tail/musttail/notail flags on CallInsts
Dávid Bolvanský via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 13 10:51:43 PDT 2021
xbolva00 added inline comments.
================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2435-2444
if (Value *With = Simplifier.optimizeCall(CI, Builder)) {
+ if (CI->isMustTailCall()) {
+ auto *NewCI = dyn_cast<CallInst>(With);
+ assert(NewCI && "replaced musttail call with non-CallInst");
+ assert(NewCI->isMustTailCall() &&
+ "replaced musttail call with non-musttail call");
+ (void)NewCI;
----------------
nickdesaulniers wrote:
> xbolva00 wrote:
> > nickdesaulniers wrote:
> > > Thinking about this overnight...if this is the one place where replacement occurs, then why don't I just copy over the tail call kind here, rather than use the adapter in multiple locations in LibCallSimplifier? As I've already demonstrating, this approach is error prone; perhaps I could "do the work" here. Then we wouldn't need the asserts here, and this becomes a smaller change. Let me give that a shot and see if there's something I'm missing here as to why that can't be done.
> > This is what was the idea of https://reviews.llvm.org/D89249 but check @efriedma 's comment. First point is problematic.
> >
> > For second and third:
> >
> > "A lot of combines create calls, but return some other value."
> >
> > I think this is not a issue. We simply fail to preserve this info, but what is problematic .. musttail. I still think if CI->isMustTailCall() == true, than we should throw away NewCI if NewCI is not a CallInst.
> >
> > "For the floating-point routines, we probably want to add "tail" unconditionally."
> >
> > Yeah, probably okay and can be here. Not sure how to correctly detect it, maybe TLI.has(rountine) && rountine->getType() is FP?
> >
> >
> > I think this is not a issue. We simply fail to preserve this info, but what is problematic .. musttail. I still think if CI->isMustTailCall() == true, than we should throw away NewCI if NewCI is not a CallInst.
>
> I agree. Let me comment on D89249; after spending too much time looking into this, I think that is the right approach. With some modifications to D89249, I think we should land that, then I'll rebase all these tests I've added onto that. Sound good?
Yeah,, this approach sounds better.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107872/new/
https://reviews.llvm.org/D107872
More information about the llvm-commits
mailing list