[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

[Evan Cheng]

Hi Nuno,

I have to agree with Duncan on this. The instcombine transformation seems wrong to me. Can you re-implement it in a different pass that can modify cfg?

Evan

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
> 
> thanks for explaining.  You should really add a testcase testing this: the one
> in the patch just seems to check invoke of undef and invoke of null.  (The lack
> of a testcase was the reason I wasn't sure what you were doing).
> 
>>> 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.
> 
>>> Finally, doing thus funky intrinsic thing seems a bizarre approach to me,
>>> there has to be a better way.
>>> 
>>> I think plenty of people made these remarks already, but I didn't notice
>>> you answering those objections - maybe I just missed it?
>> 
>> So let first explain why I proposed this approach. Instcombine cannot modify the
>> CFG, but removing an invoke instruction modifies the CFG.  The problem now is
>> what do I do if I want to remove an invoke statement in instcombine?  You may
>> say you don't.  But then you cannot optimize a bunch of things, including the
>> elimination of useless 'new' calls as I described above.
>> So then I came with the idea of inserting a dummy no-op intrinsic, which will be
>> removed by SimplifyCFG.
> 
> The fact you have to jump through hoops in instcombine to do this just tells me
> that you shouldn't be doing it in instcombine.  Please do it somewhere else.
> SimplifyLibCalls would be logical, since this is really a libcall optimization,
> other there is SimplifyCFG.  There's also PruneEH!
> 
> Ciao, Duncan.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits