<div dir="ltr">I reverted this in r255562 based on Hans's analysis.</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Dec 14, 2015 at 1:33 PM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Mon, Dec 14, 2015 at 1:25 PM, Reid Kleckner via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> This change does not appear to respect the comments here:<br>
> // If AlternativeV is not nullptr, we care about both incoming values in<br>
> PHI.<br>
> // PHI must be exactly: phi <ty> [ %BB, %V ], [ %OtherBB, %AlternativeV]<br>
> // where OtherBB is the single other predecessor of BB's only successor.<br>
><br>
> I suspect this change broke some Chromium tests. I'm in the process of<br>
> reproducing the failure and testing if a revert fixes things.<br>
<br>
</span>Reverting locally fixes the Chromium test for me.<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
> On Mon, Dec 14, 2015 at 2:57 AM, James Molloy via llvm-commits<br>
> <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> Author: jamesm<br>
>> Date: Mon Dec 14 04:57:01 2015<br>
>> New Revision: 255489<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=255489&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=255489&view=rev</a><br>
>> Log:<br>
>> Don't create unnecessary PHIs<br>
>><br>
>> In conditional store merging, we were creating PHIs when we didn't<br>
>> need to. If the value to be predicated isn't defined in the block<br>
>> we're predicating, then it doesn't need a PHI at all (because we only<br>
>> deal with triangles and diamonds, any value not in the predicated BB<br>
>> must dominate the predicated BB).<br>
>><br>
>> This fixes a large code size increase in some benchmarks in a popular<br>
>> embedded benchmark suite.<br>
>><br>
>> Added:<br>
>> llvm/trunk/test/Transforms/SimplifyCFG/merge-cond-stores-2.ll<br>
>> Modified:<br>
>> llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp<br>
>> llvm/trunk/test/Transforms/SimplifyCFG/merge-cond-stores.ll<br>
>><br>
>> Modified: llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp?rev=255489&r1=255488&r2=255489&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp?rev=255489&r1=255488&r2=255489&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp (original)<br>
>> +++ llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp Mon Dec 14 04:57:01<br>
>> 2015<br>
>> @@ -2385,6 +2385,11 @@ static Value *ensureValueAvailableInSucc<br>
>> // If AlternativeV is not nullptr, we care about both incoming values<br>
>> in PHI.<br>
>> // PHI must be exactly: phi <ty> [ %BB, %V ], [ %OtherBB,<br>
>> %AlternativeV]<br>
>> // where OtherBB is the single other predecessor of BB's only<br>
>> successor.<br>
>> +<br>
>> + // If V is not an instruction defined in BB, just return it.<br>
>> + if (!isa<Instruction>(V) || cast<Instruction>(V)->getParent() != BB)<br>
>> + return V;<br>
>> +<br>
>> PHINode *PHI = nullptr;<br>
>> BasicBlock *Succ = BB->getSingleSuccessor();<br>
>><br>
>><br>
>> Added: llvm/trunk/test/Transforms/SimplifyCFG/merge-cond-stores-2.ll<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/merge-cond-stores-2.ll?rev=255489&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/merge-cond-stores-2.ll?rev=255489&view=auto</a><br>
>><br>
>> ==============================================================================<br>
>> --- llvm/trunk/test/Transforms/SimplifyCFG/merge-cond-stores-2.ll (added)<br>
>> +++ llvm/trunk/test/Transforms/SimplifyCFG/merge-cond-stores-2.ll Mon Dec<br>
>> 14 04:57:01 2015<br>
>> @@ -0,0 +1,198 @@<br>
>> +; RUN: opt -S < %s -simplifycfg -simplifycfg-merge-cond-stores=true<br>
>> -simplifycfg-merge-cond-stores-aggressively=false<br>
>> -phi-node-folding-threshold=2 | FileCheck %s<br>
>> +<br>
>> +target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"<br>
>> +target triple = "armv7--linux-gnueabihf"<br>
>> +<br>
>> +; This is a bit reversal that has been run through the early optimizer<br>
>> (-mem2reg -gvn -instcombine).<br>
>> +; There should be no additional PHIs created at all. The store should be<br>
>> on its own in a predicated<br>
>> +; block and there should be no PHIs.<br>
>> +<br>
>> +; CHECK-LABEL: @f<br>
>> +; CHECK-NOT: phi<br>
>> +; CHECK: br i1 {{.*}}, label %[[L:.*]], label %[[R:.*]]<br>
>> +; CHECK: [[L]] ; preds =<br>
>> +; CHECK-NEXT: store<br>
>> +; CHECK-NEXT: br label %[[R]]<br>
>> +; CHECK: [[R]] ; preds =<br>
>> +; CHECK-NEXT: ret i32 0<br>
>> +<br>
>> +define i32 @f(i32* %b) {<br>
>> +entry:<br>
>> + %0 = load i32, i32* %b, align 4<br>
>> + %and = and i32 %0, 1<br>
>> + %tobool = icmp eq i32 %and, 0<br>
>> + br i1 %tobool, label %if.end, label %if.then<br>
>> +<br>
>> +if.then: ; preds = %entry<br>
>> + %or = or i32 %0, -2147483648<br>
>> + store i32 %or, i32* %b, align 4<br>
>> + br label %if.end<br>
>> +<br>
>> +if.end: ; preds = %entry,<br>
>> %if.then<br>
>> + %1 = phi i32 [ %0, %entry ], [ %or, %if.then ]<br>
>> + %and1 = and i32 %1, 2<br>
>> + %tobool2 = icmp eq i32 %and1, 0<br>
>> + br i1 %tobool2, label %if.end5, label %if.then3<br>
>> +<br>
>> +if.then3: ; preds = %if.end<br>
>> + %or4 = or i32 %1, 1073741824<br>
>> + store i32 %or4, i32* %b, align 4<br>
>> + br label %if.end5<br>
>> +<br>
>> +if.end5: ; preds = %if.end,<br>
>> %if.then3<br>
>> + %2 = phi i32 [ %1, %if.end ], [ %or4, %if.then3 ]<br>
>> + %and6 = and i32 %2, 4<br>
>> + %tobool7 = icmp eq i32 %and6, 0<br>
>> + br i1 %tobool7, label %if.end10, label %if.then8<br>
>> +<br>
>> +if.then8: ; preds = %if.end5<br>
>> + %or9 = or i32 %2, 536870912<br>
>> + store i32 %or9, i32* %b, align 4<br>
>> + br label %if.end10<br>
>> +<br>
>> +if.end10: ; preds = %if.end5,<br>
>> %if.then8<br>
>> + %3 = phi i32 [ %2, %if.end5 ], [ %or9, %if.then8 ]<br>
>> + %and11 = and i32 %3, 8<br>
>> + %tobool12 = icmp eq i32 %and11, 0<br>
>> + br i1 %tobool12, label %if.end15, label %if.then13<br>
>> +<br>
>> +if.then13: ; preds = %if.end10<br>
>> + %or14 = or i32 %3, 268435456<br>
>> + store i32 %or14, i32* %b, align 4<br>
>> + br label %if.end15<br>
>> +<br>
>> +if.end15: ; preds = %if.end10,<br>
>> %if.then13<br>
>> + %4 = phi i32 [ %3, %if.end10 ], [ %or14, %if.then13 ]<br>
>> + %and16 = and i32 %4, 16<br>
>> + %tobool17 = icmp eq i32 %and16, 0<br>
>> + br i1 %tobool17, label %if.end20, label %if.then18<br>
>> +<br>
>> +if.then18: ; preds = %if.end15<br>
>> + %or19 = or i32 %4, 134217728<br>
>> + store i32 %or19, i32* %b, align 4<br>
>> + br label %if.end20<br>
>> +<br>
>> +if.end20: ; preds = %if.end15,<br>
>> %if.then18<br>
>> + %5 = phi i32 [ %4, %if.end15 ], [ %or19, %if.then18 ]<br>
>> + %and21 = and i32 %5, 32<br>
>> + %tobool22 = icmp eq i32 %and21, 0<br>
>> + br i1 %tobool22, label %if.end25, label %if.then23<br>
>> +<br>
>> +if.then23: ; preds = %if.end20<br>
>> + %or24 = or i32 %5, 67108864<br>
>> + store i32 %or24, i32* %b, align 4<br>
>> + br label %if.end25<br>
>> +<br>
>> +if.end25: ; preds = %if.end20,<br>
>> %if.then23<br>
>> + %6 = phi i32 [ %5, %if.end20 ], [ %or24, %if.then23 ]<br>
>> + %and26 = and i32 %6, 64<br>
>> + %tobool27 = icmp eq i32 %and26, 0<br>
>> + br i1 %tobool27, label %if.end30, label %if.then28<br>
>> +<br>
>> +if.then28: ; preds = %if.end25<br>
>> + %or29 = or i32 %6, 33554432<br>
>> + store i32 %or29, i32* %b, align 4<br>
>> + br label %if.end30<br>
>> +<br>
>> +if.end30: ; preds = %if.end25,<br>
>> %if.then28<br>
>> + %7 = phi i32 [ %6, %if.end25 ], [ %or29, %if.then28 ]<br>
>> + %and31 = and i32 %7, 256<br>
>> + %tobool32 = icmp eq i32 %and31, 0<br>
>> + br i1 %tobool32, label %if.end35, label %if.then33<br>
>> +<br>
>> +if.then33: ; preds = %if.end30<br>
>> + %or34 = or i32 %7, 8388608<br>
>> + store i32 %or34, i32* %b, align 4<br>
>> + br label %if.end35<br>
>> +<br>
>> +if.end35: ; preds = %if.end30,<br>
>> %if.then33<br>
>> + %8 = phi i32 [ %7, %if.end30 ], [ %or34, %if.then33 ]<br>
>> + %and36 = and i32 %8, 512<br>
>> + %tobool37 = icmp eq i32 %and36, 0<br>
>> + br i1 %tobool37, label %if.end40, label %if.then38<br>
>> +<br>
>> +if.then38: ; preds = %if.end35<br>
>> + %or39 = or i32 %8, 4194304<br>
>> + store i32 %or39, i32* %b, align 4<br>
>> + br label %if.end40<br>
>> +<br>
>> +if.end40: ; preds = %if.end35,<br>
>> %if.then38<br>
>> + %9 = phi i32 [ %8, %if.end35 ], [ %or39, %if.then38 ]<br>
>> + %and41 = and i32 %9, 1024<br>
>> + %tobool42 = icmp eq i32 %and41, 0<br>
>> + br i1 %tobool42, label %if.end45, label %if.then43<br>
>> +<br>
>> +if.then43: ; preds = %if.end40<br>
>> + %or44 = or i32 %9, 2097152<br>
>> + store i32 %or44, i32* %b, align 4<br>
>> + br label %if.end45<br>
>> +<br>
>> +if.end45: ; preds = %if.end40,<br>
>> %if.then43<br>
>> + %10 = phi i32 [ %9, %if.end40 ], [ %or44, %if.then43 ]<br>
>> + %and46 = and i32 %10, 2048<br>
>> + %tobool47 = icmp eq i32 %and46, 0<br>
>> + br i1 %tobool47, label %if.end50, label %if.then48<br>
>> +<br>
>> +if.then48: ; preds = %if.end45<br>
>> + %or49 = or i32 %10, 1048576<br>
>> + store i32 %or49, i32* %b, align 4<br>
>> + br label %if.end50<br>
>> +<br>
>> +if.end50: ; preds = %if.end45,<br>
>> %if.then48<br>
>> + %11 = phi i32 [ %10, %if.end45 ], [ %or49, %if.then48 ]<br>
>> + %and51 = and i32 %11, 4096<br>
>> + %tobool52 = icmp eq i32 %and51, 0<br>
>> + br i1 %tobool52, label %if.end55, label %if.then53<br>
>> +<br>
>> +if.then53: ; preds = %if.end50<br>
>> + %or54 = or i32 %11, 524288<br>
>> + store i32 %or54, i32* %b, align 4<br>
>> + br label %if.end55<br>
>> +<br>
>> +if.end55: ; preds = %if.end50,<br>
>> %if.then53<br>
>> + %12 = phi i32 [ %11, %if.end50 ], [ %or54, %if.then53 ]<br>
>> + %and56 = and i32 %12, 8192<br>
>> + %tobool57 = icmp eq i32 %and56, 0<br>
>> + br i1 %tobool57, label %if.end60, label %if.then58<br>
>> +<br>
>> +if.then58: ; preds = %if.end55<br>
>> + %or59 = or i32 %12, 262144<br>
>> + store i32 %or59, i32* %b, align 4<br>
>> + br label %if.end60<br>
>> +<br>
>> +if.end60: ; preds = %if.end55,<br>
>> %if.then58<br>
>> + %13 = phi i32 [ %12, %if.end55 ], [ %or59, %if.then58 ]<br>
>> + %and61 = and i32 %13, 16384<br>
>> + %tobool62 = icmp eq i32 %and61, 0<br>
>> + br i1 %tobool62, label %if.end65, label %if.then63<br>
>> +<br>
>> +if.then63: ; preds = %if.end60<br>
>> + %or64 = or i32 %13, 131072<br>
>> + store i32 %or64, i32* %b, align 4<br>
>> + br label %if.end65<br>
>> +<br>
>> +if.end65: ; preds = %if.end60,<br>
>> %if.then63<br>
>> + %14 = phi i32 [ %13, %if.end60 ], [ %or64, %if.then63 ]<br>
>> + %and66 = and i32 %14, 32768<br>
>> + %tobool67 = icmp eq i32 %and66, 0<br>
>> + br i1 %tobool67, label %if.end70, label %if.then68<br>
>> +<br>
>> +if.then68: ; preds = %if.end65<br>
>> + %or69 = or i32 %14, 65536<br>
>> + store i32 %or69, i32* %b, align 4<br>
>> + br label %if.end70<br>
>> +<br>
>> +if.end70: ; preds = %if.end65,<br>
>> %if.then68<br>
>> + %15 = phi i32 [ %14, %if.end65 ], [ %or69, %if.then68 ]<br>
>> + %and71 = and i32 %15, 128<br>
>> + %tobool72 = icmp eq i32 %and71, 0<br>
>> + br i1 %tobool72, label %if.end75, label %if.then73<br>
>> +<br>
>> +if.then73: ; preds = %if.end70<br>
>> + %or74 = or i32 %15, 16777216<br>
>> + store i32 %or74, i32* %b, align 4<br>
>> + br label %if.end75<br>
>> +<br>
>> +if.end75: ; preds = %if.end70,<br>
>> %if.then73<br>
>> + ret i32 0<br>
>> +}<br>
>><br>
>> Modified: llvm/trunk/test/Transforms/SimplifyCFG/merge-cond-stores.ll<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/merge-cond-stores.ll?rev=255489&r1=255488&r2=255489&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/merge-cond-stores.ll?rev=255489&r1=255488&r2=255489&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- llvm/trunk/test/Transforms/SimplifyCFG/merge-cond-stores.ll (original)<br>
>> +++ llvm/trunk/test/Transforms/SimplifyCFG/merge-cond-stores.ll Mon Dec 14<br>
>> 04:57:01 2015<br>
>> @@ -2,10 +2,8 @@<br>
>><br>
>> ; CHECK-LABEL: @test_simple<br>
>> ; This test should succeed and end up if-converted.<br>
>> -; CHECK: icmp eq i32 %b, 0<br>
>> -; CHECK-NEXT: icmp ne i32 %a, 0<br>
>> -; CHECK-NEXT: xor i1 %x2, true<br>
>> -; CHECK-NEXT: %[[x:.*]] = or i1 %{{.*}}, %{{.*}}<br>
>> +; CHECK: %[[A:.*]] = or i32 %a, %b<br>
>> +; CHECK-NEXT: %[[x:.*]] = icmp eq i32 %[[A]], 0<br>
>> ; CHECK-NEXT: br i1 %[[x]]<br>
>> ; CHECK: store<br>
>> ; CHECK-NOT: store<br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
><br>
</div></div></blockquote></div><br></div>