[PATCH] D16337: [LibCallSimplifier] fold memset(malloc(x), 0, x) --> calloc(1, x)

[Chad Rosier via llvm-commits]

mcrosier accepted this revision.
mcrosier added a comment.
This revision is now accepted and ready to land.

LGTM assuming you've done the necessary correctness/performance due diligence.


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:942
@@ +941,3 @@
+                               const TargetLibraryInfo &TLI) {
+  // This has to be a memset of zeros (bzero).
+  auto *FillValue = dyn_cast<ConstantInt>(Memset->getArgOperand(1));
----------------
spatel wrote:
> mcrosier wrote:
> > Would it make more sense to check for calloc here?
> > 
> > if (!TLI.getLibFunc("calloc", Func) || !TLI.has(Func))
> >     return nullptr;
> > 
> > It would avoid some unnecessary work in the event we don't have calloc.  However, I assume we generally have calloc, so this might not be a great suggestion...  Just a suggestion that doesn't necessarily have to be acted upon.
> If we view this in isolation, that sounds reasonable, but the calloc existence check is in emitCalloc because that's the pattern used by the other emit* functions in BuildLibCalls. 
> 
> I'd prefer to leave this as-is pending resolution of the 'TODO' at line 918 about moving emitCalloc() over to BuildLibCalls to be with its siblings (even though that entire separate file is currently unnecessary because it's only ever used here AFAICT).
> 
> It's also possible (there's always a chance!) that someone will come up with some other transform here in LibCallSimplifier that needs a calloc(). In that case, I think it'd also be better to leave the existence check where it is.
All very reasonable comments, which is why I left this to your discretion.


http://reviews.llvm.org/D16337