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

[Duncan Sands]

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.  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
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>