[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 Chris Lattner <clattner at apple.com>:

> On Jun 26, 2012, at 12:12 AM, Duncan Sands wrote:
>> Hi Nuno,
>>
>>>>>  - instcombine:  invoke new  -> invoke expect(0, 0)  (an arbitrary NOOP
>>>>> intrinsic;  only done if the allocated memory is unused, of course)
>>>>
>>>> What is this about?  If you are saying that malloc is nounwind,  
>>>> why aren't you
>>>> marking it nounwind in SimplifyLibCalls.
>>>
>>> What the code is doing is deleting sequences like:
>>> p = new foo;
>>> .. no real uses of p..
>>> delete p; // optional
>
> I think this sort of thing is an important case that instcombine  
> *should* be able to optimize.  It is sad that we can't do this  
> today.  However, I also don't like Nuno's first try at this. :)
>
>>>> In any case, by removing the restriction that you can't do invoke on an
>>>> intrinsic you are making a major (historically speaking change).   
>>>> It should
>>>> thus be done as part of a different commit, and accompanied with  
>>>> a bunch of
>>>> tests checking that codegen doesn't crash if you try to codegen code that
>>>> does invoke of an intrinsic.
>>
>> You didn't address this point.
>
> This is also the part I don't like.  After thinking about this a  
> bit, the only way I can see to sanely handle this is to have  
> instcombine zap the alloc/delete pair.  Since it can't (and we don't  
> want it to) modify the CFG, I think that it is reasonable for  
> instcombine to zap the alloc by changing the invoke to call a noop  
> intrinsic.
>
> That said, I don't agree that all intrinsics should be valid in an  
> invoke, and I don't agree that the expect intrinsic should be used  
> for this purpose.  Nuno, in  separate patches, how about you do the  
> following:
>
> 1. Introduce an new llvm.undef.result intrinsics, which can be used  
> with arbitrary arguments and return value.  It is fully defined to  
> ignore its argument and return undef.  Codegen should be enhanced to  
> handle it by lowering it to nothing (the results would be lowered to  
> IMPLICIT_DEFs if not dead).
>
> 2. Enhance the verifier etc to allow llvm.undef.result *only* to be  
> used in an invoke instruction.  All the other intrinsics should not  
> be allowed there.  Make sure (i.e. add a test) that codegen handles  
> the intrinsic if it gets to codegen in an invoke.
>
> 3. Teach simplifycfg to zap invoke llvm.undef.result.
>
> 4. Enhance instcombine to do the optimization by "zapping" the alloc  
> by replacing it with an llvm.undef.result.
>
> I'm not super thrilled with the name "llvm.undef.result", but I  
> don't like the name "llvm.noop" (which could be reasonably expected  
> to generate a hardware nop instruction) and don't want to call it  
> "llvm.undef" because it will be confusing and because it is  
> perfectly well defined as an operation, it just produces an  
> undefined result.
>
> Does this seem reasonable?
>
> As a side note, the real villain here is the invoke instruction.   
> Someday we should really kill it  
> (http://nondot.org/sabre/LLVMNotes/ExceptionHandlingChanges.txt) :)
>
> -Chris


Sounds good to me. Start working on it now!
Nuno