Hi Nuno,
>>> make simplifyCFG erase invokes to readonly/readnone functions
>>
>> this is wrong, please don't do this! Being readonly/readnone is orthogonal to
>> being nounwind.
>
> Sorry, I guess my commit log was not explicit enough.
> I'm just removing invokes to functions that are both readnone and nounwind. The
> code doesn't touch functions that may unwind.
oof, glad to hear that! However I'm surprised that this changes anything,
since the invoke will be turned into a call (because of nounwind), then the
call will be deleted (because calls of readonly functions are removed if the
result is not used). Or are you trying to get the same effect more efficiently?
> My reasoning was a bit simpler than yours (maybe naive, though), which was that
> if a function can throw an exception, then we cannot delete a call to it, since
> then we'll never possibly see that exception, meaning that we changed the
> semantics of the program.
Correct. For calls, they are removed if !mayHaveSideEffects, where
bool mayHaveSideEffects() const {
return mayWriteToMemory() || mayThrow();
}
mayThrow means !nounwind, while mayWriteToMemory for a call instruction means
!readonly.
Ciao, Duncan.
> BTW, thanks for your explanation! Always learning :)
>
> Nuno
>
>
>> To convince you of this, consider the following simple way that
>> some basic exception handling could be implemented: every function returns an
>> extra boolean value. If "true" is returned in it (indicating that an exception
>> was thrown) then the caller branches to the appropriate code: either a landing
>> pad if called from an invoke, or, if an ordinary call was used, to code that
>> returns "true" in the caller's boolean (i.e. continues to unwind).
>>
>> Notice how in this scheme it is perfectly possible for a function to be readnone
>> but throw an exception (i.e. return "true").
>>
>> Note that these extra return values don't have to be present at the IR level,
>> codegen can add them as its way of implementing exception handling. In fact
>> it might be nice if codegen offered this scheme as an option, for languages
>> that don't need all the dwarf bells and whistles. In fact vmkit does this
>> (I don't know if it does it at the IR level or via codegen) because it is
>> faster for java (which throws exceptions a lot) than using dwarf.
>>
>> It is true that the dwarf implementation of exception handling writes to memory
>> all over the place, and thus is incompatible with readonly/readnone. But that's
>> just due to how the dwarf implementation works, and thus shouldn't be baked into
>> the IR as your patch does.
>>
>> In short, at the IR level it would be wrong to do this, because it is
>> presupposing how codegen will implement exception handling. Maybe it is OK to
>> do this optimization at the codegen level once you know that dwarf exception
>> handling has been selected?
>>
>> Ciao, Duncan.
>>
>> PS: a front-end can always output nounwind on functions it knows are
>> readnone/readonly if it wants those semantics.
>> PPS: this was endlessly debated on the GCC mailing list, and the conclusion for
>> LLVM was to decouple readnone/readonly and nounwind, which lets front-ends make
>> their own decision.
>>
>>>
>>> Modified:
>>> llvm/trunk/lib/Transforms/Scalar/SimplifyCFGPass.cpp
>>> llvm/trunk/test/Transforms/SimplifyCFG/invoke.ll
>>>
>>> Modified: llvm/trunk/lib/Transforms/Scalar/SimplifyCFGPass.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/SimplifyCFGPass.cpp?rev=159385&r1=159384&r2=159385&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/Transforms/Scalar/SimplifyCFGPass.cpp (original)
>>> +++ llvm/trunk/lib/Transforms/Scalar/SimplifyCFGPass.cpp Thu Jun 28 17:32:27
>>> 2012
>>> @@ -89,7 +89,6 @@
>>>
>>> /// ChangeToCall - Convert the specified invoke into a normal call.
>>> static void ChangeToCall(InvokeInst *II) {
>>> - BasicBlock *BB = II->getParent();
>>> SmallVector<Value*, 8> Args(II->op_begin(), II->op_end() - 3);
>>> CallInst *NewCall = CallInst::Create(II->getCalledValue(), Args, "", II);
>>> NewCall->takeName(II);
>>> @@ -100,10 +99,7 @@
>>>
>>> // Follow the call by a branch to the normal destination.
>>> BranchInst::Create(II->getNormalDest(), II);
>>> -
>>> - // Update PHI nodes in the unwind destination
>>> - II->getUnwindDest()->removePredecessor(BB);
>>> - BB->getInstList().erase(II);
>>> + II->eraseFromParent();
>>> }
>>>
>>> static bool MarkAliveBlocks(BasicBlock *BB,
>>> @@ -163,7 +159,12 @@
>>> ChangeToUnreachable(II, true);
>>> Changed = true;
>>> } else if (II->doesNotThrow()) {
>>> - ChangeToCall(II);
>>> + if (II->use_empty() && II->onlyReadsMemory()) {
>>> + // jump to the normal destination branch.
>>> + BranchInst::Create(II->getNormalDest(), II);
>>> + II->eraseFromParent();
>>> + } else
>>> + ChangeToCall(II);
>>> Changed = true;
>>> }
>>> }
>>>
>>> Modified: llvm/trunk/test/Transforms/SimplifyCFG/invoke.ll
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/invoke.ll?rev=159385&r1=159384&r2=159385&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/test/Transforms/SimplifyCFG/invoke.ll (original)
>>> +++ llvm/trunk/test/Transforms/SimplifyCFG/invoke.ll Thu Jun 28 17:32:27 2012
>>> @@ -3,7 +3,7 @@
>>>
>>> declare i32 @__gxx_personality_v0(...)
>>> declare void @__cxa_call_unexpected(i8*)
>>> -declare i64 @llvm.objectsize.i64(i8*, i1) nounwind readonly
>>> +declare i32 @read_only() nounwind readonly
>>>
>>>
>>> ; CHECK: @f1
>>> @@ -43,3 +43,42 @@
>>> tail call void @__cxa_call_unexpected(i8* %1) noreturn nounwind
>>> unreachable
>>> }
>>> +
>>> +; CHECK: @f3
>>> +define i32 @f3() nounwind uwtable ssp {
>>> +; CHECK-NEXT: entry
>>> +entry:
>>> +; CHECK-NEXT: ret i32 3
>>> + %call = invoke i32 @read_only()
>>> + to label %invoke.cont unwind label %lpad
>>> +
>>> +invoke.cont:
>>> + ret i32 3
>>> +
>>> +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: @f4
>>> +define i32 @f4() nounwind uwtable ssp {
>>> +; CHECK-NEXT: entry
>>> +entry:
>>> +; CHECK-NEXT: call i32 @read_only()
>>> + %call = invoke i32 @read_only()
>>> + to label %invoke.cont unwind label %lpad
>>> +
>>> +invoke.cont:
>>> +; CHECK-NEXT: ret i32 %call
>>> + ret i32 %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
>>> +}