[PATCH] instcombine: Only create library call simplifier once

[Michael Ilseman]

Thanks for looking into this. I have some questions/concerns.

On Mar 7, 2013, at 12:52 PM, Meador Inge <meadori at codesourcery.com> wrote:

> Nadav reported a performance regression due to the work I did to
> merge the library call simplifier into instcombine [1].  The issue
> is that a new LibCallSimplifier object is being created every time
> InstCombiner::runOnFunction is called.  The is very inefficient
> because every time a LibCallSimplifier object is new'd up it creates
> a hash table to map from a function name to an object that optimizes
> functions of that name.
> 

Actually, I think only one was new-ed up. Also, it was the "LibCallSimplifierImpl::optimizeCall()" method which would call "initOptimizations()" only if Optimizations wasn't already set up. I believe (though I'm possibly wrong) that this was only happening once per function. Once per function is still unfortunate, as Nadav pointed out, for small C++ wrapper functions.

> The original library call simplifier avoided this problem by only
> initializing that table *once* in SimplifyLibCalls::runOnFunction.
> This patch fixes the problem by implementing a similar idea in
> instcombine.  LibCallSimplifier objects are only created the first
> time InstCombiner::runOnFunction is called.

Did you do any measurements, and if so what was the data? The hash table was made inside LibCallSimplifierImpl::optimizeCall(), not in its constructor. I haven't tried the patch, but I don't see where the intended speedup is coming form. If I'm missing something obvious, feel free to point it out :)

More comments below:

> 
> OK?
> 
> [1] http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130304/167639.html
> ---
> include/llvm/Transforms/Utils/SimplifyLibCalls.h    |  4 ++--
> lib/Transforms/InstCombine/InstCombine.h            |  5 ++++-
> lib/Transforms/InstCombine/InstCombineCalls.cpp     |  2 +-
> lib/Transforms/InstCombine/InstructionCombining.cpp |  9 ++++-----
> lib/Transforms/Utils/SimplifyLibCalls.cpp           | 17 +++++++----------
> 5 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/include/llvm/Transforms/Utils/SimplifyLibCalls.h b/include/llvm/Transforms/Utils/SimplifyLibCalls.h
> index 6bb81be..c636e1a 100644
> --- a/include/llvm/Transforms/Utils/SimplifyLibCalls.h
> +++ b/include/llvm/Transforms/Utils/SimplifyLibCalls.h
> @@ -31,7 +31,7 @@ namespace llvm {
>     /// simplifier.
>     LibCallSimplifierImpl *Impl;
>   public:
> -    LibCallSimplifier(const DataLayout *TD, const TargetLibraryInfo *TLI,
> +    LibCallSimplifier(const TargetLibraryInfo *TLI,
>                       bool UnsafeFPShrink);
>     virtual ~LibCallSimplifier();
> 
> @@ -41,7 +41,7 @@ namespace llvm {
>     /// be equal to the instruction being optimized.  In this case all
>     /// other instructions that use the given instruction were modified
>     /// and the given instruction is dead.
> -    Value *optimizeCall(CallInst *CI);
> +    Value *optimizeCall(CallInst *CI, const DataLayout *TD);
> 
>     /// replaceAllUsesWith - This method is used when the library call
>     /// simplifier needs to replace instructions other than the library
> diff --git a/lib/Transforms/InstCombine/InstCombine.h b/lib/Transforms/InstCombine/InstCombine.h
> index 1f6a3a5e..6ec3786 100644
> --- a/lib/Transforms/InstCombine/InstCombine.h
> +++ b/lib/Transforms/InstCombine/InstCombine.h
> @@ -87,11 +87,14 @@ public:
>   BuilderTy *Builder;
> 
>   static char ID; // Pass identification, replacement for typeid
> -  InstCombiner() : FunctionPass(ID), TD(0), Builder(0) {
> +  InstCombiner() : FunctionPass(ID), TD(0), Simplifier(0), Builder(0) {
>     MinimizeSize = false;
>     initializeInstCombinerPass(*PassRegistry::getPassRegistry());
>   }
> 
> +  ~InstCombiner() {
> +    delete Simplifier;
> +  }

See below note about releaseMemory()

> public:
>   virtual bool runOnFunction(Function &F);
> 
> diff --git a/lib/Transforms/InstCombine/InstCombineCalls.cpp b/lib/Transforms/InstCombine/InstCombineCalls.cpp
> index 64cd1bd..848a790 100644
> --- a/lib/Transforms/InstCombine/InstCombineCalls.cpp
> +++ b/lib/Transforms/InstCombine/InstCombineCalls.cpp
> @@ -790,7 +790,7 @@ static bool isSafeToEliminateVarargsCast(const CallSite CS,
> Instruction *InstCombiner::tryOptimizeCall(CallInst *CI, const DataLayout *TD) {
>   if (CI->getCalledFunction() == 0) return 0;
> 
> -  if (Value *With = Simplifier->optimizeCall(CI)) {
> +  if (Value *With = Simplifier->optimizeCall(CI, TD)) {
>     ++NumSimplified;
>     return CI->use_empty() ? CI : ReplaceInstUsesWith(*CI, With);
>   }
> diff --git a/lib/Transforms/InstCombine/InstructionCombining.cpp b/lib/Transforms/InstCombine/InstructionCombining.cpp
> index c6115e3..327c70c 100644
> --- a/lib/Transforms/InstCombine/InstructionCombining.cpp
> +++ b/lib/Transforms/InstCombine/InstructionCombining.cpp
> @@ -2456,10 +2456,9 @@ namespace {
> class InstCombinerLibCallSimplifier : public LibCallSimplifier {
>   InstCombiner *IC;
> public:
> -  InstCombinerLibCallSimplifier(const DataLayout *TD,
> -                                const TargetLibraryInfo *TLI,
> +  InstCombinerLibCallSimplifier(const TargetLibraryInfo *TLI,
>                                 InstCombiner *IC)
> -    : LibCallSimplifier(TD, TLI, UnsafeFPShrink) {
> +    : LibCallSimplifier(TLI, UnsafeFPShrink) {
>     this->IC = IC;
>   }
> 
> @@ -2485,8 +2484,8 @@ bool InstCombiner::runOnFunction(Function &F) {
>                InstCombineIRInserter(Worklist));
>   Builder = &TheBuilder;
> 
> -  InstCombinerLibCallSimplifier TheSimplifier(TD, TLI, this);
> -  Simplifier = &TheSimplifier;
> +  if (!Simplifier)
> +    Simplifier = new InstCombinerLibCallSimplifier(TLI, this);
> 

Are you sure you want to keep pointers live across multiple compilations? I would think you would have to destroy this in releaseMemory(). I'm also not sure what's gained here, the constructor is fairly trivial.

>   bool EverMadeChange = false;
> 
> diff --git a/lib/Transforms/Utils/SimplifyLibCalls.cpp b/lib/Transforms/Utils/SimplifyLibCalls.cpp
> index 8ff87ee..29cece4 100644
> --- a/lib/Transforms/Utils/SimplifyLibCalls.cpp
> +++ b/lib/Transforms/Utils/SimplifyLibCalls.cpp
> @@ -1668,7 +1668,6 @@ struct PutsOpt : public LibCallOptimization {
> namespace llvm {
> 
> class LibCallSimplifierImpl {
> -  const DataLayout *TD;
>   const TargetLibraryInfo *TLI;
>   const LibCallSimplifier *LCS;
>   bool UnsafeFPShrink;
> @@ -1728,18 +1727,17 @@ class LibCallSimplifierImpl {
>   void addOpt(LibFunc::Func F, LibCallOptimization* Opt);
>   void addOpt(LibFunc::Func F1, LibFunc::Func F2, LibCallOptimization* Opt);
> public:
> -  LibCallSimplifierImpl(const DataLayout *TD, const TargetLibraryInfo *TLI,
> +  LibCallSimplifierImpl(const TargetLibraryInfo *TLI,
>                         const LibCallSimplifier *LCS,
>                         bool UnsafeFPShrink = false)
>     : UnaryDoubleFP(false), UnsafeUnaryDoubleFP(true),
>       Cos(UnsafeFPShrink), Pow(UnsafeFPShrink), Exp2(UnsafeFPShrink) {
> -    this->TD = TD;
>     this->TLI = TLI;
>     this->LCS = LCS;
>     this->UnsafeFPShrink = UnsafeFPShrink;
>   }
> 
> -  Value *optimizeCall(CallInst *CI);
> +  Value *optimizeCall(CallInst *CI, const DataLayout *TD);
> };
> 
> void LibCallSimplifierImpl::initOptimizations() {
> @@ -1854,7 +1852,7 @@ void LibCallSimplifierImpl::initOptimizations() {
>   addOpt(LibFunc::puts, &Puts);
> }
> 
> -Value *LibCallSimplifierImpl::optimizeCall(CallInst *CI) {
> +Value *LibCallSimplifierImpl::optimizeCall(CallInst *CI, const DataLayout *TD) {
>   if (Optimizations.empty())
>     initOptimizations();
> 
> @@ -1878,19 +1876,18 @@ void LibCallSimplifierImpl::addOpt(LibFunc::Func F1, LibFunc::Func F2,
>     Optimizations[TLI->getName(F1)] = Opt;
> }
> 
> -LibCallSimplifier::LibCallSimplifier(const DataLayout *TD,
> -                                     const TargetLibraryInfo *TLI,
> +LibCallSimplifier::LibCallSimplifier(const TargetLibraryInfo *TLI,
>                                      bool UnsafeFPShrink) {
> -  Impl = new LibCallSimplifierImpl(TD, TLI, this, UnsafeFPShrink);
> +  Impl = new LibCallSimplifierImpl(TLI, this, UnsafeFPShrink);
> }
> 
> LibCallSimplifier::~LibCallSimplifier() {
>   delete Impl;
> }
> 
> -Value *LibCallSimplifier::optimizeCall(CallInst *CI) {
> +Value *LibCallSimplifier::optimizeCall(CallInst *CI, const DataLayout *TD) {
>   if (CI->hasFnAttr(Attribute::NoBuiltin)) return 0;
> -  return Impl->optimizeCall(CI);
> +  return Impl->optimizeCall(CI, TD);
> }
> 
> void LibCallSimplifier::replaceAllUsesWith(Instruction *I, Value *With) const {
> -- 
> 1.8.1
>