<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On May 9, 2016, at 10:23 AM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" class="">spatel@rotateright.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">Oops, I recently added that code to CGP and forgot the '0' check. I'll make the patch.<br class=""></div></div></blockquote>Cool. Thanks!</div><div><br class=""></div><div>BTW, here’s the reduced test case:</div><div><br class=""></div><div>target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"<br class="">target triple = "x86_64-apple-macosx10.10.0"<br class=""><br class="">define i32 @foo(i32 %true, i32 %false, i8 %x) align 2 {<br class="">entry:<br class=""> %cmp.i.i.i.i.3 = icmp eq i8 %x, 58<br class=""> %.mux = select i1 %cmp.i.i.i.i.3, i32 %true, i32 %false, !prof !33<br class=""> ret i32 %.mux<br class="">}<br class=""><br class="">!33 = !{!"branch_weights", i32 0, i32 0}</div><div><br class=""></div><div>Cheers,</div><div>Pete<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><br class="">So yes, branch weights of 0 are valid, and we just need to add a check that if 'Sum' is zero, then don't try to make a BranchProb there.<br class=""></div><div class="gmail_extra"><br class=""><div class="gmail_quote">On Mon, May 9, 2016 at 11:15 AM, Pete Cooper <span dir="ltr" class=""><<a href="mailto:peter_cooper@apple.com" target="_blank" class="">peter_cooper@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On May 9, 2016, at 9:20 AM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank" class="">spatel@rotateright.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class="">Thanks, Pete. A quick skim of BranchProbability users shows that we don't always check for the '0' weight denominator possibility, so hopefully it's an easy fix.<br class=""></div></div></blockquote></span>I don’t know anything about the probabilities, but are you saying that 0 isn’t necessarily incorrect, just that it needs to be checked for?</div><div class=""><br class=""></div><div class="">I’m looking at the IR for the failure now to reduce it. What we have is a select instruction with !prof equal to !{!"branch_weights", i32 0, i32 0}</div><div class=""><br class=""></div><div class="">That then goes to CGP isFormingBranchFromSelectProfitable and crashes in here:</div><div class=""><br class=""></div><div class=""> if (SI->extractProfMetadata(TrueWeight, FalseWeight)) {<br class=""> uint64_t Max = std::max(TrueWeight, FalseWeight);<br class=""> uint64_t Sum = TrueWeight + FalseWeight;<br class=""> auto Probability = BranchProbability::getBranchProbability(Max, Sum);<br class=""> if (Probability > TLI->getPredictableBranchThreshold())<br class=""> return true;<br class=""> }</div><div class=""><br class=""></div><div class="">If all we need to do is check for a 0 here then thats something I can write up a patch for.</div><div class=""><br class=""></div><div class="">Thanks for all the help on this!</div><div class=""><span class="HOEnZb"><font color="#888888" class="">Pete</font></span><div class=""><div class="h5"><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Sat, May 7, 2016 at 11:00 PM, Pete Cooper <span dir="ltr" class=""><<a href="mailto:peter_cooper@apple.com" target="_blank" class="">peter_cooper@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto" class=""><div class="">Hi Sanjay</div><div class=""><br class=""></div><div class="">Our internal bot did indeed start passing after I reverted this. Looks like it doesn't like the change for some reason.</div><div class=""><br class=""></div><div class="">I'll try get a reduced bitcode on Monday to diagnose it.</div><div class=""><br class=""></div><div class="">Cheers</div><div class="">Pete<br class=""><br class="">Sent from my iPhone</div><div class=""><div class=""><div class=""><br class="">On May 6, 2016, at 5:07 PM, Pete Cooper <<a href="mailto:peter_cooper@apple.com" target="_blank" class="">peter_cooper@apple.com</a>> wrote:<br class=""><br class=""></div><blockquote type="cite" class=""><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On May 6, 2016, at 3:56 PM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank" class="">spatel@rotateright.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class=""><div class=""><div class="">For the record, it looks like this iteration of the patch has made it past the troubled bot:<br class=""><a href="http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/11639" target="_blank" class="">http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/11639</a><br class=""></div>...and after staring at the code, I still don't see how this version could be logically different than the previous rev.</div></div></div></blockquote><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div>But I don't think I've hit my personal record for reverts of a single patch, so if we want to revert this while investigating the assert, I don't object. :)<br class=""></div></div></blockquote>Thanks, i appreciate that.</div><div class=""><br class=""></div><div class="">As this is only affecting our internal bots, i’ll revert there and kick a build. Will let you know on Monday if that really does find the issue.</div><div class=""><br class=""></div><div class="">Cheers,</div><div class="">Pete<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div></div><div class="gmail_extra"><br class=""><div class="gmail_quote">On Fri, May 6, 2016 at 4:44 PM, Sanjay Patel <span dir="ltr" class=""><<a href="mailto:spatel@rotateright.com" target="_blank" class="">spatel@rotateright.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class=""><div class="">Hi Pete,<br class=""><br class="">It certainly seems related to branch weights.<br class=""><br class=""></div>I actually haven't looked at what happens to the metadata downstream from here. Do you know if the assertion is triggered in an IR optimizer pass or the backend? <br class=""></div><div class=""><div class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Fri, May 6, 2016 at 4:25 PM, Xinliang David Li <span dir="ltr" class=""><<a href="mailto:xinliangli@gmail.com" target="_blank" class="">xinliangli@gmail.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="">It is likely caused some other existing problems that get triggered by the change.<span class=""><font color="#888888" class=""><div class=""><br class=""></div><div class="">David</div></font></span></div><div class=""><div class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Fri, May 6, 2016 at 2:59 PM, Pete Cooper via llvm-commits <span dir="ltr" class=""><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Sanjay<br class="">
<br class="">
I’m seeing a failure on our internal bots which may be caused by this. I’m not actually sure how to proceed. Do you have any idea if this could have been caused by your change? It was the only one in the list that looks like it may have done so.<br class="">
<br class="">
The error is:<br class="">
<br class="">
/Users/buildslave/jenkins/sharedspace/apple-clang-stage2-RA@2/clang.roots/BuildRecords/clang-9999.99_install/Objects/obj-llvm/./bin/clang++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/Target/X86 -I/Users/buildslave/jenkins/sharedspace/apple-clang-stage2-RA@2/clang/src/lib/Target/X86 -Iinclude -I/Users/buildslave/jenkins/sharedspace/apple-clang-stage2-RA@2/clang/src/include -fno-stack-protector -fno-common -Wno-profile-instr-unprofiled -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Werror=date-time -std=c++11 -fcolor-diagnostics -O2 -gline-tables-only -arch x86_64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk -mmacosx-version-min=10.10 -UNDEBUG -fprofile-instr-use=/Users/buildslave/jenkins/sharedspace/apple-clang-stage2-RA@2/clang.roots/BuildRecords/clang-9999.99_install/Objects/clang.profdata -fno-exceptions -fno-rtti -MMD -MT lib/Target/X86/CMakeFiles/LLVMX86CodeGen.dir/X86TargetTransformInfo.cpp.o -MF lib/Target/X86/CMakeFiles/LLVMX86CodeGen.dir/X86TargetTransformInfo.cpp.o.d -o lib/Target/X86/CMakeFiles/LLVMX86CodeGen.dir/X86TargetTransformInfo.cpp.o -c '/Users/buildslave/jenkins/sharedspace/apple-clang-stage2-RA@2/clang/src/lib/Target/X86/X86TargetTransformInfo.cpp'<br class="">
<br class="">
warning: profile data may be out of date: of 251 functions, 2 have no data and 43 have mismatched data that will be ignored [-Wprofile-instr-out-of-date]<br class="">
<br class="">
Assertion failed: (Denominator > 0 && "Denominator cannot be 0!"), function BranchProbability, file /Users/buildslave/jenkins/sharedspace/apple-clang-stage2-RA@2/clang/src/lib/Support/BranchProbability.cpp, line 38.<br class="">
<br class="">
Cheers,<br class="">
Pete<br class="">
<div class=""><div class="">> On May 6, 2016, at 11:07 AM, Sanjay Patel via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>> wrote:<br class="">
><br class="">
> Author: spatel<br class="">
> Date: Fri May 6 13:07:46 2016<br class="">
> New Revision: 268767<br class="">
><br class="">
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=268767&view=rev" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project?rev=268767&view=rev</a><br class="">
> Log:<br class="">
> [SimplifyCFG] propagate branch metadata when creating select (retry r268550 / r268751 with possible fix)<br class="">
><br class="">
> Retrying r268550/r268751 which were reverted at r268577/r268765 due a memory sanitizer failure.<br class="">
> I have not been able to reproduce that failure, but I've taken another guess at fixing<br class="">
> the problem in this version of the patch and will watch for another failure.<br class="">
><br class="">
> Original commit message:<br class="">
> Unlike earlier similar fixes, we need to recalculate the branch weights<br class="">
> in this case.<br class="">
><br class="">
> Differential Revision: <a href="http://reviews.llvm.org/D19674" rel="noreferrer" target="_blank" class="">http://reviews.llvm.org/D19674</a><br class="">
><br class="">
><br class="">
> Modified:<br class="">
> llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp<br class="">
> llvm/trunk/test/Transforms/SimplifyCFG/preserve-branchweights.ll<br class="">
><br class="">
> Modified: llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp<br class="">
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp?rev=268767&r1=268766&r2=268767&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp?rev=268767&r1=268766&r2=268767&view=diff</a><br class="">
> ==============================================================================<br class="">
> --- llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp (original)<br class="">
> +++ llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp Fri May 6 13:07:46 2016<br class="">
> @@ -2860,9 +2860,29 @@ static bool SimplifyCondBranchToCondBran<br class="">
> Value *PBIV = PN->getIncomingValue(PBBIdx);<br class="">
> if (BIV != PBIV) {<br class="">
> // Insert a select in PBI to pick the right value.<br class="">
> - Value *NV = cast<SelectInst><br class="">
> + SelectInst *NV = cast<SelectInst><br class="">
> (Builder.CreateSelect(PBICond, PBIV, BIV, PBIV->getName() + ".mux"));<br class="">
> PN->setIncomingValue(PBBIdx, NV);<br class="">
> + // Although the select has the same condition as PBI, the original branch<br class="">
> + // weights for PBI do not apply to the new select because the select's<br class="">
> + // 'logical' edges are incoming edges of the phi that is eliminated, not<br class="">
> + // the outgoing edges of PBI.<br class="">
> + if (PredHasWeights && SuccHasWeights) {<br class="">
> + uint64_t PredCommon = PBIOp ? PredFalseWeight : PredTrueWeight;<br class="">
> + uint64_t PredOther = PBIOp ? PredTrueWeight : PredFalseWeight;<br class="">
> + uint64_t SuccCommon = BIOp ? SuccFalseWeight : SuccTrueWeight;<br class="">
> + uint64_t SuccOther = BIOp ? SuccTrueWeight : SuccFalseWeight;<br class="">
> + // The weight to PredCommonDest should be PredCommon * SuccTotal.<br class="">
> + // The weight to PredOtherDest should be PredOther * SuccCommon.<br class="">
> + uint64_t NewWeights[2] = {PredCommon * (SuccCommon + SuccOther),<br class="">
> + PredOther * SuccCommon};<br class="">
> +<br class="">
> + FitWeights(NewWeights);<br class="">
> +<br class="">
> + NV->setMetadata(LLVMContext::MD_prof,<br class="">
> + MDBuilder(BI->getContext())<br class="">
> + .createBranchWeights(NewWeights[0], NewWeights[1]));<br class="">
> + }<br class="">
> }<br class="">
> }<br class="">
><br class="">
><br class="">
> Modified: llvm/trunk/test/Transforms/SimplifyCFG/preserve-branchweights.ll<br class="">
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/preserve-branchweights.ll?rev=268767&r1=268766&r2=268767&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/preserve-branchweights.ll?rev=268767&r1=268766&r2=268767&view=diff</a><br class="">
> ==============================================================================<br class="">
> --- llvm/trunk/test/Transforms/SimplifyCFG/preserve-branchweights.ll (original)<br class="">
> +++ llvm/trunk/test/Transforms/SimplifyCFG/preserve-branchweights.ll Fri May 6 13:07:46 2016<br class="">
> @@ -412,22 +412,48 @@ return:<br class="">
> ret i32 %retval.0<br class="">
> }<br class="">
><br class="">
> -; The 1st select should have branch weights equal to the 1st branch.<br class="">
> -; The 2nd select should have freshly calculated branch weights.<br class="">
> +; The selects should have freshly calculated branch weights.<br class="">
><br class="">
> define i32 @SimplifyCondBranchToCondBranch(i1 %cmpa, i1 %cmpb) {<br class="">
> ; CHECK-LABEL: @SimplifyCondBranchToCondBranch(<br class="">
> ; CHECK-NEXT: block1:<br class="">
> -; CHECK-NEXT: [[BRMERGE:%.*]] = or i1 %cmpb, %cmpa<br class="">
> -; CHECK-NEXT: [[DOTMUX:%.*]] = select i1 %cmpb, i32 0, i32 2<br class="">
> -; CHECK-NEXT: [[OUTVAL:%.*]] = select i1 [[BRMERGE]], i32 [[DOTMUX]], i32 1, !prof !12<br class="">
> +; CHECK-NEXT: [[BRMERGE:%.*]] = or i1 %cmpa, %cmpb<br class="">
> +; CHECK-NEXT: [[DOTMUX:%.*]] = select i1 %cmpa, i32 0, i32 2, !prof !12<br class="">
> +; CHECK-NEXT: [[OUTVAL:%.*]] = select i1 [[BRMERGE]], i32 [[DOTMUX]], i32 1, !prof !13<br class="">
> ; CHECK-NEXT: ret i32 [[OUTVAL]]<br class="">
> ;<br class="">
> block1:<br class="">
> - br i1 %cmpb, label %block3, label %block2, !prof !0<br class="">
> + br i1 %cmpa, label %block3, label %block2, !prof !13<br class="">
><br class="">
> block2:<br class="">
> - br i1 %cmpa, label %block3, label %exit, !prof !2<br class="">
> + br i1 %cmpb, label %block3, label %exit, !prof !14<br class="">
> +<br class="">
> +block3:<br class="">
> + %cowval = phi i32 [ 2, %block2 ], [ 0, %block1 ]<br class="">
> + br label %exit<br class="">
> +<br class="">
> +exit:<br class="">
> + %outval = phi i32 [ %cowval, %block3 ], [ 1, %block2 ]<br class="">
> + ret i32 %outval<br class="">
> +}<br class="">
> +<br class="">
> +; Swap the operands of the compares to verify that the weights update correctly.<br class="">
> +<br class="">
> +define i32 @SimplifyCondBranchToCondBranchSwap(i1 %cmpa, i1 %cmpb) {<br class="">
> +; CHECK-LABEL: @SimplifyCondBranchToCondBranchSwap(<br class="">
> +; CHECK-NEXT: block1:<br class="">
> +; CHECK-NEXT: [[CMPA_NOT:%.*]] = xor i1 %cmpa, true<br class="">
> +; CHECK-NEXT: [[CMPB_NOT:%.*]] = xor i1 %cmpb, true<br class="">
> +; CHECK-NEXT: [[BRMERGE:%.*]] = or i1 [[CMPA_NOT]], [[CMPB_NOT]]<br class="">
> +; CHECK-NEXT: [[DOTMUX:%.*]] = select i1 [[CMPA_NOT]], i32 0, i32 2, !prof !14<br class="">
> +; CHECK-NEXT: [[OUTVAL:%.*]] = select i1 [[BRMERGE]], i32 [[DOTMUX]], i32 1, !prof !15<br class="">
> +; CHECK-NEXT: ret i32 [[OUTVAL]]<br class="">
> +;<br class="">
> +block1:<br class="">
> + br i1 %cmpa, label %block2, label %block3, !prof !13<br class="">
> +<br class="">
> +block2:<br class="">
> + br i1 %cmpb, label %exit, label %block3, !prof !14<br class="">
><br class="">
> block3:<br class="">
> %cowval = phi i32 [ 2, %block2 ], [ 0, %block1 ]<br class="">
> @@ -452,6 +478,8 @@ exit:<br class="">
> !10 = !{!"branch_weights", i32 672646, i32 21604207}<br class="">
> !11 = !{!"branch_weights", i32 6960, i32 21597248}<br class="">
> !12 = !{!"these_are_not_the_branch_weights_you_are_looking_for", i32 3, i32 5}<br class="">
> +!13 = !{!"branch_weights", i32 2, i32 3}<br class="">
> +!14 = !{!"branch_weights", i32 4, i32 7}<br class="">
><br class="">
> ; CHECK: !0 = !{!"branch_weights", i32 5, i32 11}<br class="">
> ; CHECK: !1 = !{!"branch_weights", i32 1, i32 5}<br class="">
> @@ -467,5 +495,8 @@ exit:<br class="">
> ;; treat the weight as an unsigned integer.<br class="">
> ; CHECK: !10 = !{!"branch_weights", i32 112017436, i32 -735157296}<br class="">
> ; CHECK: !11 = !{!"branch_weights", i32 3, i32 5}<br class="">
> -; CHECK: !12 = !{!"branch_weights", i32 14, i32 10}<br class="">
> +; CHECK: !12 = !{!"branch_weights", i32 22, i32 12}<br class="">
> +; CHECK: !13 = !{!"branch_weights", i32 34, i32 21}<br class="">
> +; CHECK: !14 = !{!"branch_weights", i32 33, i32 14}<br class="">
> +; CHECK: !15 = !{!"branch_weights", i32 47, i32 8}<br class="">
><br class="">
><br class="">
><br class="">
> _______________________________________________<br class="">
> llvm-commits mailing list<br class="">
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a><br class="">
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br class="">
<br class="">
_______________________________________________<br class="">
llvm-commits mailing list<br class="">
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a><br class="">
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br class="">
</div></div></blockquote></div><br class=""></div>
</div></div></blockquote></div><br class=""></div>
</div></div></blockquote></div><br class=""></div>
</div></blockquote></div><br class=""></div></blockquote></div></div></div></blockquote></div><br class=""></div>
</div></blockquote></div></div></div><br class=""></div></blockquote></div><br class=""></div>
</div></blockquote></div><br class=""></body></html>