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

[Eli Friedman]

On Sun, Mar 20, 2011 at 11:53 AM, Frits van Bommel <fvbommel at gmail.com> wrote:
> 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 worth pointing out that the CS could be an invoke.  (It wouldn't
be very useful because __cxa_atexit can't throw, but not illegal
AFAIK.)

-Eli

> Also, note that 'CS.getInstruction()->' is equivalent to 'CS->' due to
> operator overloading :).
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>