[llvm-commits] [llvm] r159385 - in /llvm/trunk: lib/Transforms/Scalar/SimplifyCFGPass.cpp test/Transforms/SimplifyCFG/invoke.ll

[Nuno Lopes]

Quoting Duncan Sands <baldrick at free.fr>:

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

:)
Yes, the idea was to make it more efficient. And really make sure the  
calls are deleted (I spotted a case where it wasn't, possibly due to  
pass ordering issues).

Nuno


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