[PATCH] D107872: [SimplifyLibCalls] propagate tail flag on FORITIFY calls

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:3516
+    if (CI->isTailCall() && !SimplifiedCI->isTailCall())
+      SimplifiedCI->setTailCall();
+
----------------
efriedma wrote:
> xbolva00 wrote:
> > nickdesaulniers wrote:
> > > efriedma wrote:
> > > > This isn't the right call in some cases, I think.  For example, for memmove, this is the operand to the memmove, not the memmove itself.
> > > Yeah, I don't quite understand not returning the newly constructed call instruction.  Does that mean that the call to `replaceAllUsesWith` on line 3041 wont actually insert the new call instruction, or rather replace the old instruction? 
> > memmove/fortified memmove returns a pointer, llvm.memmove and friends return “void”
> The code at line 3041 is probably not working as intended, yes.
> 
> Actually, I'm not sure what that code is trying to do; not sure why we don't want to just let the instcombine worklist do its normal thing.
Ah, looks like there's seperate test files for a few of these.
* memcpy_chk-1.ll, memcpy_chk-2.ll
* memset_chk-1.ll, memset_chk-2.ll
* stpcpy_chk-1.ll
* strcpy_chk-64.ll, strcpy_chk-2.ll

Also, in https://bugs.llvm.org/show_bug.cgi?id=46734, it looks like whatever converts calls to `__memcpy_chk` to `@llvm.memcpy.p0i8.p0i8.i64` is also dropping these!!!

Oh god, it's endemic to the non-chk/vanilla LibCallSimplifier, too!  Should I litter the code with these, or should we consider running `TailCallElim` immediately after `LibcallSimplifier`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107872