[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

[Duncan Sands]

Hi,

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

currently all intrinsics are marked nounwind (when I implemented that I removed
all the special "intrinsics can't unwind" logic at the IR level), so even if
you did an invoke on them it would be turned into a call by the optimizers.  It
would probably be easy at the codegen levels to turn invoke-of-nounwind-
function into a call, for the -O0 case.  That would make invoke of intrinsics
safe.  If one day we want an intrinsic that can unwind, it would be easy enough
to add by not marking it nounwind.  In short, I think it would be simple enough
to remove the restriction that you can't do invoke on an intrinsic.

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

For some time I've wanted an llvm.identity intrinsic, which is basically just
an "undef" barrier.  The concept is that, given x, the result of
llvm.identity(x) is just x, however llvm.identity(undef) is something without
the funky undef semantics, for example 0.  This is useful for range checks.
Anyway, such an intrinsic would be generally useful, and could also be used
here rather than introducing llvm.undef.result, just use llvm.identity(0).

Ciao, Duncan.

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