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

[Anders Carlsson]

On Mar 20, 2011, at 11:53 AM, Frits van Bommel wrote:

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

Fixed.

> 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)

Nope, I don't think we want them to be deleted. I added a FIXME for the readonly/readnone and nounwind case.

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

Fixed!

>> +    } 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.

I intentionally wanted to make this as simple as possible to only catch what llvm-gcc and clang does.

> 
> 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.

Added a check for an integer return type and used RUAW.

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

Doh! Fixed. (Everything is fixed in r127974). Thanks for the review!

- Anders
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110320/a142a23f/attachment.html>