<div dir="ltr">Thanks Pete. Committed in r<span style="font-size:12.800000190734863px">281419 and r</span><span style="font-size:12.800000190734863px">281421.</span><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 13, 2016 at 3:29 PM, Pete Cooper <span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Akira<br>
<br>
Just a single nit. Can you please define *OrigArg on its own line as its easier to read.<br>
<br>
Otherwise LGTM. I’m surprised we didn’t come across this issue sooner!<br>
<br>
Cheers,<br>
Pete<br>
<div><div class="h5">> On Sep 13, 2016, at 2:55 PM, Akira Hatanaka <<a href="mailto:ahatanak@gmail.com">ahatanak@gmail.com</a>> wrote:<br>
><br>
> ahatanak created this revision.<br>
> ahatanak added reviewers: gottesmm, pete, rjmccall.<br>
> ahatanak added a subscriber: llvm-commits.<br>
><br>
> ObjCARCContract::runOnFunction tries to replace uses of an argument passed to an objective-c library call with the call<br>
> return value. For example, in the following IR, it replaces uses of argument %9 and uses of the values discovered traversing the chain upwards (%7 and %8) with the call return %10, if they are dominated by the call to @objc_autoreleaseReturnValue. This transformation enables code-gen to tail-call the call to @objc_autoreleaseReturnValue, which is necessary to enable auto release return value optimization.<br>
><br>
> ```<br>
> %7 = tail call i8* @objc_loadWeakRetained(i8** %6)<br>
> %8 = bitcast i8* %7 to %0*<br>
> %9 = bitcast %0* %8 to i8*<br>
> %10 = tail call i8* @objc_autoreleaseReturnValue(<wbr>i8* %9)<br>
> ret %0* %8<br>
> ```<br>
><br>
> Since r276727, llvm started removing redundant bitcasts and as a result started feeding the following IR to ARC contraction:<br>
><br>
> ```<br>
> %7 = tail call i8* @objc_loadWeakRetained(i8** %6)<br>
> %8 = bitcast i8* %7 to %0*<br>
> %9 = tail call i8* @objc_autoreleaseReturnValue(<wbr>i8* %7)<br>
> ret %0* %8<br>
> ```<br>
><br>
> ARC contraction no longer does the optimization described above since it only traverses the chain upwards and fails to recognize that the function return can be replaced by the call return from @objc_autoreleaseReturnValue. This patch changes ObjCARCContract::runOnFunction to traverse the chain downwards too and replace uses of bitcasts with the call return.<br>
><br>
> rdar://problem/28011339<br>
><br>
> <a href="https://reviews.llvm.org/D24523" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D24523</a><br>
><br>
> Files:<br>
> lib/Transforms/ObjCARC/<wbr>ObjCARCContract.cpp<br>
> test/Transforms/ObjCARC/<wbr>contract-replace-arg-use.ll<br>
><br>
> Index: test/Transforms/ObjCARC/<wbr>contract-replace-arg-use.ll<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- /dev/null<br>
> +++ test/Transforms/ObjCARC/<wbr>contract-replace-arg-use.ll<br>
> @@ -0,0 +1,18 @@<br>
> +; RUN: opt -objc-arc-contract -S < %s | FileCheck %s<br>
> +<br>
> +declare i8* @objc_autoreleaseReturnValue(<wbr>i8*)<br>
> +declare i8* @foo1()<br>
> +<br>
> +; Check that ARC contraction replaces the function return with the value<br>
> +; returned by @objc_autoreleaseReturnValue.<br>
> +<br>
> +; CHECK: %[[V0:[0-9]+]] = tail call i8* @objc_autoreleaseReturnValue(<br>
> +; CHECK: %[[V1:[0-9]+]] = bitcast i8* %[[V0]] to i32*<br>
> +; CHECK: ret i32* %[[V1]]<br>
> +<br>
> +define i32* @autoreleaseRVTailCall() {<br>
> + %1 = call i8* @foo1()<br>
> + %2 = bitcast i8* %1 to i32*<br>
> + %3 = tail call i8* @objc_autoreleaseReturnValue(<wbr>i8* %1)<br>
> + ret i32* %2<br>
> +}<br>
> Index: lib/Transforms/ObjCARC/<wbr>ObjCARCContract.cpp<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- lib/Transforms/ObjCARC/<wbr>ObjCARCContract.cpp<br>
> +++ lib/Transforms/ObjCARC/<wbr>ObjCARCContract.cpp<br>
> @@ -547,13 +547,13 @@<br>
><br>
> // Don't use GetArgRCIdentityRoot because we don't want to look through bitcasts<br>
> // and such; to do the replacement, the argument must have type i8*.<br>
> - Value *Arg = cast<CallInst>(Inst)-><wbr>getArgOperand(0);<br>
><br>
> - // TODO: Change this to a do-while.<br>
> - for (;;) {<br>
> + // Function for replacing uses of Arg dominated by Inst.<br>
> + auto ReplaceArgUses = [Inst, this](Value *Arg) {<br>
> // If we're compiling bugpointed code, don't get in trouble.<br>
> if (!isa<Instruction>(Arg) && !isa<Argument>(Arg))<br>
> - break;<br>
> + return;<br>
> +<br>
> // Look through the uses of the pointer.<br>
> for (Value::use_iterator UI = Arg->use_begin(), UE = Arg->use_end();<br>
> UI != UE; ) {<br>
> @@ -598,6 +598,14 @@<br>
> }<br>
> }<br>
> }<br>
> + };<br>
> +<br>
> +<br>
> + Value *Arg = cast<CallInst>(Inst)-><wbr>getArgOperand(0), *OrigArg = Arg;<br>
> +<br>
> + // TODO: Change this to a do-while.<br>
> + for (;;) {<br>
> + ReplaceArgUses(Arg);<br>
><br>
> // If Arg is a no-op casted pointer, strip one level of casts and iterate.<br>
> if (const BitCastInst *BI = dyn_cast<BitCastInst>(Arg))<br>
> @@ -611,6 +619,24 @@<br>
> else<br>
> break;<br>
> }<br>
> +<br>
> + // Replace bitcast users of Arg that are dominated by Inst.<br>
> + SmallVector<BitCastInst *, 2> BitCastUsers;<br>
> +<br>
> + // Add all bitcast users of the function argument first.<br>
> + for (User *U : OrigArg->users())<br>
> + if (auto *BC = dyn_cast<BitCastInst>(U))<br>
> + BitCastUsers.push_back(BC);<br>
> +<br>
> + // Replace the bitcasts with the call return. Iterate until list is empty.<br>
> + while (!BitCastUsers.empty()) {<br>
> + auto *BC = BitCastUsers.pop_back_val();<br>
> + for (User *U : BC->users())<br>
> + if (auto *B = dyn_cast<BitCastInst>(U))<br>
> + BitCastUsers.push_back(B);<br>
> +<br>
> + ReplaceArgUses(BC);<br>
> + }<br>
> }<br>
><br>
> // If this function has no escaping allocas or suspicious vararg usage,<br>
><br>
><br>
</div></div>> <D24523.71239.patch><br>
<br>
</blockquote></div><br></div></div>