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

Hi Duncan,

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

That's exactly the same transformation we already do for malloc (in  
instcombine). This transformation has nothing to do with saying that  
'new' is nounwind. I don't even think that you can perform such a  
transformation soundly: new may always unwind.


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

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? Which option do you think it's better?

And no, noone else complained so far, but maybe because no one else  
read the patch I sent last week or today's commit..

Nuno


> Ciao, Duncan.
>
>>   - verifier:  allow invoke of intrinsics  (to make the previous step work)
>>
>> Added:
>>      llvm/trunk/test/Transforms/SimplifyCFG/invoke.ll
>> Modified:
>>      llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp
>>      llvm/trunk/lib/Transforms/Scalar/SimplifyCFGPass.cpp
>>      llvm/trunk/lib/VMCore/Verifier.cpp
>>      llvm/trunk/test/Transforms/InstCombine/objsize-64.ll
>>
>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp
>> URL:  
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp?rev=159146&r1=159145&r2=159146&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp  
>> (original)
>> +++ llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp  
>> Mon Jun 25 12:11:47 2012
>> @@ -1169,7 +1169,14 @@
>>       }
>>
>>       if (InvokeInst *II = dyn_cast<InvokeInst>(&MI)) {
>> -      BranchInst::Create(II->getNormalDest(), II->getParent());
>> +      // Replace invoke with a NOOP intrinsic to maintain the original CFG
>> +      Module *M = II->getParent()->getParent()->getParent();
>> +      IntegerType *Ty = IntegerType::get(II->getContext(), 8);
>> +      ConstantInt *CI = ConstantInt::get(Ty, 0);
>> +      Value *Args[] = {CI, CI};
>> +      Function *F = Intrinsic::getDeclaration(M, Intrinsic::expect, Ty);
>> +      InvokeInst::Create(F, II->getNormalDest(), II->getUnwindDest(), Args,
>> +                         "dummy", II->getParent());
>>       }
>>       return EraseInstFromFunction(MI);
>>     }
>>
>> Modified: llvm/trunk/lib/Transforms/Scalar/SimplifyCFGPass.cpp
>> URL:  
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/SimplifyCFGPass.cpp?rev=159146&r1=159145&r2=159146&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Scalar/SimplifyCFGPass.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Scalar/SimplifyCFGPass.cpp Mon Jun 25  
>> 12:11:47 2012
>> @@ -157,11 +157,16 @@
>>       }
>>
>>       // Turn invokes that call 'nounwind' functions into ordinary calls.
>> -    if (InvokeInst *II = dyn_cast<InvokeInst>(BB->getTerminator()))
>> -      if (II->doesNotThrow()) {
>> +    if (InvokeInst *II = dyn_cast<InvokeInst>(BB->getTerminator())) {
>> +      Value *Callee = II->getCalledValue();
>> +      if (isa<ConstantPointerNull>(Callee) || isa<UndefValue>(Callee)) {
>> +        ChangeToUnreachable(II, true);
>> +        Changed = true;
>> +      } else if (II->doesNotThrow()) {
>>           ChangeToCall(II);
>>           Changed = true;
>>         }
>> +    }
>>
>>       Changed |= ConstantFoldTerminator(BB, true);
>>       for (succ_iterator SI = succ_begin(BB), SE = succ_end(BB); SI  
>> != SE; ++SI)
>>
>> Modified: llvm/trunk/lib/VMCore/Verifier.cpp
>> URL:  
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/VMCore/Verifier.cpp?rev=159146&r1=159145&r2=159146&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/VMCore/Verifier.cpp (original)
>> +++ llvm/trunk/lib/VMCore/Verifier.cpp Mon Jun 25 12:11:47 2012
>> @@ -1636,7 +1636,8 @@
>>       if (Function *F = dyn_cast<Function>(I.getOperand(i))) {
>>         // Check to make sure that the "address of" an intrinsic  
>> function is never
>>         // taken.
>> -      Assert1(!F->isIntrinsic() || (i + 1 == e && isa<CallInst>(I)),
>> +      CallSite CS(&I);
>> +      Assert1(!F->isIntrinsic() || (CS && i == (CS.isCall() ? e-1 : 2)),
>>                 "Cannot take the address of an intrinsic!", &I);
>>         Assert1(F->getParent() == Mod, "Referencing function in  
>> another module!",
>>                 &I);
>>
>> Modified: llvm/trunk/test/Transforms/InstCombine/objsize-64.ll
>> URL:  
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/objsize-64.ll?rev=159146&r1=159145&r2=159146&view=diff
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/InstCombine/objsize-64.ll (original)
>> +++ llvm/trunk/test/Transforms/InstCombine/objsize-64.ll Mon Jun 25  
>> 12:11:47 2012
>> @@ -19,7 +19,7 @@
>>   ; CHECK: @f2
>>   define i64 @f2() nounwind uwtable ssp {
>>   entry:
>> -; CHECK: br label
>> +; CHECK: invoke i8 @llvm.expect.i8(i8 0, i8 0)
>>     %call = invoke noalias i8* @_Znwm(i64 13)
>>             to label %invoke.cont unwind label %lpad
>>
>>
>> Added: llvm/trunk/test/Transforms/SimplifyCFG/invoke.ll
>> URL:  
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/invoke.ll?rev=159146&view=auto
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/SimplifyCFG/invoke.ll (added)
>> +++ llvm/trunk/test/Transforms/SimplifyCFG/invoke.ll Mon Jun 25  
>> 12:11:47 2012
>> @@ -0,0 +1,45 @@
>> +; RUN: opt < %s -simplifycfg -S | FileCheck %s
>> +target datalayout =  
>> "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
>> +
>> +declare i32 @__gxx_personality_v0(...)
>> +declare void @__cxa_call_unexpected(i8*)
>> +declare i64 @llvm.objectsize.i64(i8*, i1) nounwind readonly
>> +
>> +
>> +; CHECK: @f1
>> +define i8* @f1() nounwind uwtable ssp {
>> +entry:
>> +; CHECK: call void @llvm.trap()
>> +; CHECK: unreachable
>> +  %call = invoke noalias i8* undef()
>> +          to label %invoke.cont unwind label %lpad
>> +
>> +invoke.cont:
>> +  ret i8* %call
>> +
>> +lpad:
>> +  %0 = landingpad { i8*, i32 } personality i8* bitcast (i32 (...)*  
>> @__gxx_personality_v0 to i8*)
>> +          filter [0 x i8*] zeroinitializer
>> +  %1 = extractvalue { i8*, i32 } %0, 0
>> +  tail call void @__cxa_call_unexpected(i8* %1) noreturn nounwind
>> +  unreachable
>> +}
>> +
>> +; CHECK: @f2
>> +define i8* @f2() nounwind uwtable ssp {
>> +entry:
>> +; CHECK: call void @llvm.trap()
>> +; CHECK: unreachable
>> +  %call = invoke noalias i8* null()
>> +          to label %invoke.cont unwind label %lpad
>> +
>> +invoke.cont:
>> +  ret i8* %call
>> +
>> +lpad:
>> +  %0 = landingpad { i8*, i32 } personality i8* bitcast (i32 (...)*  
>> @__gxx_personality_v0 to i8*)
>> +          filter [0 x i8*] zeroinitializer
>> +  %1 = extractvalue { i8*, i32 } %0, 0
>> +  tail call void @__cxa_call_unexpected(i8* %1) noreturn nounwind
>> +  unreachable
>> +}