[llvm-commits] [llvm] r159146 - in /llvm/trunk: lib/Transforms/InstCombine/InstructionCombining.cpp lib/Transforms/Scalar/SimplifyCFGPass.cpp lib/VMCore/Verifier.cpp test/Transforms/InstCombine/objsize-64.ll test/Transforms/SimplifyCFG/invoke.ll

[Nuno Lopes]

Quoting Chandler Carruth <chandlerc at google.com>:

> On Mon, Jun 25, 2012 at 1:42 PM, Nuno Lopes <nunoplopes at sapo.pt> wrote:
>
>> I only see these 3 options: allow instcombine to modify the CFG (which
>> probably we don't want to, since it runs often and it's supposed to be
>> cheap), don't allow instcombine to touch invoke instructions ever (and
>> lose some optimization opportunities), or replace the invoke to be
>> removed with an invoke to an existing no-op function/intrinsic.
>> Any other idea?
>>
>
> See my other email for another idea. As a fifth option, we could add this
> optimization to a different pass (SimplifyLibCalls, SimplifyCFG, whatever)
> which *is* allowed to mutate the CFG. There seems no inherent reason this
> must be tied to instcombine...
>
> As a separate issue, you argue that marking the invoke as nounwind is never
> correct to do, and perhaps it isn't. However, if we cannot mark the invoke
> as nounwind, then I don't believe we can delete the e invoke either, as
> certainly once deleted it cannot unwind. ;]
>
> Essentially, we must decide: is it within as-if to remove an invoke to the
> global operator new if the result is unused? If so, then why is it not
> within as-if to mark it nounwind (not rhetorical)? Or just to remove it in
> a pass other than instcombine where we are allowed to mutate the CFG?

I think marking as nounwind is not an option because then correctness  
of the transformation will then depend on whether you run simplifyCFG  
afterwards or not. Deleting a call to 'new' is not the same as leaving  
a call to 'new' behind that is assumed to not unwind (while in fact it  
can!).


> I could see an argument that all "sane" new operators should be "elidable"
> if they are unused, but if they are not elided, they must be called and
> must have the opportunity to throw. But I don't think the standard provides
> any such concept. Perhaps it should, but in that case we need to document
> it as an extension Clang/LLVM provides. (we already have the idea of a
> 'sane' operator new fortunately, we can re-use that signal I suspect...)

I tried to decipher what the C++11 standard says about memory  
allocation, but it seems very generalist on the subject to me. I don't  
think it mandates anything that prevents this specific optimization.
Anyway, before we were already performing the very same optimization  
for malloc, which even folds the comparison between the value returned  
by a deleted malloc with null to false (basically assuming that the  
malloc call wouldn't fail).
As far as the C++ standard goes, I think this is perfectly fine.


> Given these options, my preference is for some pass other than instcombine
> to do this transform in the presence of an invoke, thus getting the
> 'elision' semantics where, if called, the operator has a chance to throw. I
> would still have instcombine do the transform when a call is directly used,
> and I would gate the transform on the 'sane new' bit we already have.

The thing is that this optimization requires a bunch of utility code  
(see Instcombine's IsOnlyNullComparedAndFreed() and friends). And so  
it would be nice to keep the call and invoke optimizations together.
I think moving all this logic to SimplifyLibCalls is not horrible  
either. I didn't considered it in the first place, because I always  
think about that pass as folding string and number manipulation  
operations (strcpy, memset, etc, etc)..
Another thing is that instcombine runs more often than  
SimplifyLibCalls (which only runs once!). So moving all this logic to  
SimplifyLibCalls may impact the optimizations. I dunno; just think out  
loud..

Nuno