[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

[Chris Lattner]

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