[llvm-commits] [llvm] r127970 - in /llvm/trunk: lib/Transforms/IPO/GlobalOpt.cpp test/Transforms/GlobalOpt/cxx-dtor.ll

[Frits van Bommel]

On Sun, Mar 20, 2011 at 6:59 PM, Anders Carlsson <andersca at mac.com> wrote:
> +static Function *FindCXAAtExit(Module &M) {
> +  Function *Fn = M.getFunction("__cxa_atexit");
> +
> +  if (!Fn)
> +    return 0;
> +
> +  const FunctionType *FTy = Fn->getFunctionType();
> +
> +  // Checking that the function has the right number of parameters and that they
> +  // all have pointer types should be enough.
> +  if (FTy->getNumParams() != 3 ||
> +      !FTy->getParamType(0)->isPointerTy() ||
> +      !FTy->getParamType(1)->isPointerTy() ||
> +      !FTy->getParamType(2)->isPointerTy())
> +    return 0;

You're not checking the return type here; see below.

> +
> +  return Fn;
> +}
> +
> +/// cxxDtorIsEmpty - Returns whether the given function is an empty C++
> +/// destructor and can therefore be eliminated.
> +/// Note that we assume that other optimization passes have already simplified
> +/// the code so we only look for a function with a single basic block, where
> +/// the only allowed instructions are 'ret' or 'call' to empty C++ dtor.
> +static bool cxxDtorIsEmpty(const Function& Fn) {
> +  if (Fn.empty())
> +    return true;

Fn.empty() is equivalent to Fn.isDeclaration(). Do you really want to
allow external destructors to be deleted?

(Note: you can allow them if they're readonly/readnone and nounwind)

> +
> +  if (++Fn.begin() != Fn.end())
> +    return false;
> +
> +  const BasicBlock &EntryBlock = Fn.getEntryBlock();
> +  for (BasicBlock::const_iterator I = EntryBlock.begin(), E = EntryBlock.end();
> +       I != E; ++I) {
> +    if (const CallInst *CI = dyn_cast<CallInst>(I)) {
> +      const Function *CalledFn = CI->getCalledFunction();
> +
> +      if (!CalledFn)
> +        return false;
> +
> +      if (!cxxDtorIsEmpty(*CalledFn))
> +        return false;

If the destructor is an infinite recursion at run-time, this turns it
into an infinite recursion at compile time...

> +    } else if (isa<ReturnInst>(*I))
> +      return true;
> +    else
> +      return false;

I think you could allow arbitrary non-terminator instructions without
side-effects here too. These could be passed as parameters to the
calls, for instance.
In fact, that check could be moved up to filter out side-effect-free
function calls too.

> +  }
> +
> +  return false;
> +}
> +
> +bool GlobalOpt::OptimizeEmptyGlobalCXXDtors(Function *CXAAtExitFn) {
> +  /// Itanium C++ ABI p3.3.5:
> +  ///
> +  ///   After constructing a global (or local static) object, that will require
> +  ///   destruction on exit, a termination function is registered as follows:
> +  ///
> +  ///   extern "C" int __cxa_atexit ( void (*f)(void *), void *p, void *d );
> +  ///
> +  ///   This registration, e.g. __cxa_atexit(f,p,d), is intended to cause the
> +  ///   call f(p) when DSO d is unloaded, before all such termination calls
> +  ///   registered before this one. It returns zero if registration is
> +  ///    successful, nonzero on failure.
> +
> +  // This pass will look for calls to __cxa_atexit where the function is trivial
> +  // and remove them.
[snip]
> +    if (!cxxDtorIsEmpty(*DtorFn))
> +      continue;
> +
> +    // Just remove the call.
> +    CS.getInstruction()->eraseFromParent();
> +    ++NumCXXDtorsRemoved;

According to the comment above, __cxa_atexit returns an int. If it has
any uses, this leaves them dangling.
Since it should be safe to pretend that registering an empty
destructor is successful, you should call
"CS->replaceAllUsesWith(Constant::getNullValue(CS.getType()));" before
erasing __cxa_atexit calls that have uses.
You should probably also either check for non-void return type in
FindCXAAtExit(), or skip the RAUW if the return type is void.

Also, note that 'CS.getInstruction()->' is equivalent to 'CS->' due to
operator overloading :).