[llvm] r268767 - [SimplifyCFG] propagate branch metadata when creating select (retry r268550 / r268751 with possible fix)
Sanjay Patel via llvm-commits
llvm-commits at lists.llvm.org
Mon May 9 09:20:06 PDT 2016
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.
On Sat, May 7, 2016 at 11:00 PM, Pete Cooper <peter_cooper at apple.com> wrote:
> Hi Sanjay
>
> Our internal bot did indeed start passing after I reverted this. Looks
> like it doesn't like the change for some reason.
>
> I'll try get a reduced bitcode on Monday to diagnose it.
>
> Cheers
> Pete
>
> Sent from my iPhone
>
> On May 6, 2016, at 5:07 PM, Pete Cooper <peter_cooper at apple.com> wrote:
>
>
> On May 6, 2016, at 3:56 PM, Sanjay Patel <spatel at rotateright.com> wrote:
>
> For the record, it looks like this iteration of the patch has made it past
> the troubled bot:
>
> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/11639
> ...and after staring at the code, I still don't see how this version could
> be logically different than the previous rev.
>
>
> 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. :)
>
> Thanks, i appreciate that.
>
> 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.
>
> Cheers,
> Pete
>
>
>
> On Fri, May 6, 2016 at 4:44 PM, Sanjay Patel <spatel at rotateright.com>
> wrote:
>
>> Hi Pete,
>>
>> It certainly seems related to branch weights.
>>
>> 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?
>>
>> On Fri, May 6, 2016 at 4:25 PM, Xinliang David Li <xinliangli at gmail.com>
>> wrote:
>>
>>> It is likely caused some other existing problems that get triggered by
>>> the change.
>>>
>>> David
>>>
>>> On Fri, May 6, 2016 at 2:59 PM, Pete Cooper via llvm-commits <
>>> llvm-commits at lists.llvm.org> wrote:
>>>
>>>> Hi Sanjay
>>>>
>>>> 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.
>>>>
>>>> The error is:
>>>>
>>>> /Users/buildslave/jenkins/sharedspace/apple-clang-stage2-RA at 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 at 2/clang/src/lib/Target/X86
>>>> -Iinclude -I/Users/buildslave/jenkins/sharedspace/apple-clang-stage2-RA at 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 at 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 at 2
>>>> /clang/src/lib/Target/X86/X86TargetTransformInfo.cpp'
>>>>
>>>> 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]
>>>>
>>>> Assertion failed: (Denominator > 0 && "Denominator cannot be 0!"),
>>>> function BranchProbability, file
>>>> /Users/buildslave/jenkins/sharedspace/apple-clang-stage2-RA at 2/clang/src/lib/Support/BranchProbability.cpp,
>>>> line 38.
>>>>
>>>> Cheers,
>>>> Pete
>>>> > On May 6, 2016, at 11:07 AM, Sanjay Patel via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>> >
>>>> > Author: spatel
>>>> > Date: Fri May 6 13:07:46 2016
>>>> > New Revision: 268767
>>>> >
>>>> > URL: http://llvm.org/viewvc/llvm-project?rev=268767&view=rev
>>>> > Log:
>>>> > [SimplifyCFG] propagate branch metadata when creating select (retry
>>>> r268550 / r268751 with possible fix)
>>>> >
>>>> > Retrying r268550/r268751 which were reverted at r268577/r268765 due a
>>>> memory sanitizer failure.
>>>> > I have not been able to reproduce that failure, but I've taken
>>>> another guess at fixing
>>>> > the problem in this version of the patch and will watch for another
>>>> failure.
>>>> >
>>>> > Original commit message:
>>>> > Unlike earlier similar fixes, we need to recalculate the branch
>>>> weights
>>>> > in this case.
>>>> >
>>>> > Differential Revision: http://reviews.llvm.org/D19674
>>>> >
>>>> >
>>>> > Modified:
>>>> > llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
>>>> > llvm/trunk/test/Transforms/SimplifyCFG/preserve-branchweights.ll
>>>> >
>>>> > Modified: llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
>>>> > URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp?rev=268767&r1=268766&r2=268767&view=diff
>>>> >
>>>> ==============================================================================
>>>> > --- llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp (original)
>>>> > +++ llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp Fri May 6
>>>> 13:07:46 2016
>>>> > @@ -2860,9 +2860,29 @@ static bool SimplifyCondBranchToCondBran
>>>> > Value *PBIV = PN->getIncomingValue(PBBIdx);
>>>> > if (BIV != PBIV) {
>>>> > // Insert a select in PBI to pick the right value.
>>>> > - Value *NV = cast<SelectInst>
>>>> > + SelectInst *NV = cast<SelectInst>
>>>> > (Builder.CreateSelect(PBICond, PBIV, BIV, PBIV->getName() +
>>>> ".mux"));
>>>> > PN->setIncomingValue(PBBIdx, NV);
>>>> > + // Although the select has the same condition as PBI, the
>>>> original branch
>>>> > + // weights for PBI do not apply to the new select because the
>>>> select's
>>>> > + // 'logical' edges are incoming edges of the phi that is
>>>> eliminated, not
>>>> > + // the outgoing edges of PBI.
>>>> > + if (PredHasWeights && SuccHasWeights) {
>>>> > + uint64_t PredCommon = PBIOp ? PredFalseWeight :
>>>> PredTrueWeight;
>>>> > + uint64_t PredOther = PBIOp ? PredTrueWeight :
>>>> PredFalseWeight;
>>>> > + uint64_t SuccCommon = BIOp ? SuccFalseWeight :
>>>> SuccTrueWeight;
>>>> > + uint64_t SuccOther = BIOp ? SuccTrueWeight : SuccFalseWeight;
>>>> > + // The weight to PredCommonDest should be PredCommon *
>>>> SuccTotal.
>>>> > + // The weight to PredOtherDest should be PredOther *
>>>> SuccCommon.
>>>> > + uint64_t NewWeights[2] = {PredCommon * (SuccCommon +
>>>> SuccOther),
>>>> > + PredOther * SuccCommon};
>>>> > +
>>>> > + FitWeights(NewWeights);
>>>> > +
>>>> > + NV->setMetadata(LLVMContext::MD_prof,
>>>> > + MDBuilder(BI->getContext())
>>>> > + .createBranchWeights(NewWeights[0],
>>>> NewWeights[1]));
>>>> > + }
>>>> > }
>>>> > }
>>>> >
>>>> >
>>>> > Modified:
>>>> llvm/trunk/test/Transforms/SimplifyCFG/preserve-branchweights.ll
>>>> > URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/preserve-branchweights.ll?rev=268767&r1=268766&r2=268767&view=diff
>>>> >
>>>> ==============================================================================
>>>> > --- llvm/trunk/test/Transforms/SimplifyCFG/preserve-branchweights.ll
>>>> (original)
>>>> > +++ llvm/trunk/test/Transforms/SimplifyCFG/preserve-branchweights.ll
>>>> Fri May 6 13:07:46 2016
>>>> > @@ -412,22 +412,48 @@ return:
>>>> > ret i32 %retval.0
>>>> > }
>>>> >
>>>> > -; The 1st select should have branch weights equal to the 1st branch.
>>>> > -; The 2nd select should have freshly calculated branch weights.
>>>> > +; The selects should have freshly calculated branch weights.
>>>> >
>>>> > define i32 @SimplifyCondBranchToCondBranch(i1 %cmpa, i1 %cmpb) {
>>>> > ; CHECK-LABEL: @SimplifyCondBranchToCondBranch(
>>>> > ; CHECK-NEXT: block1:
>>>> > -; CHECK-NEXT: [[BRMERGE:%.*]] = or i1 %cmpb, %cmpa
>>>> > -; CHECK-NEXT: [[DOTMUX:%.*]] = select i1 %cmpb, i32 0, i32 2
>>>> > -; CHECK-NEXT: [[OUTVAL:%.*]] = select i1 [[BRMERGE]], i32
>>>> [[DOTMUX]], i32 1, !prof !12
>>>> > +; CHECK-NEXT: [[BRMERGE:%.*]] = or i1 %cmpa, %cmpb
>>>> > +; CHECK-NEXT: [[DOTMUX:%.*]] = select i1 %cmpa, i32 0, i32 2,
>>>> !prof !12
>>>> > +; CHECK-NEXT: [[OUTVAL:%.*]] = select i1 [[BRMERGE]], i32
>>>> [[DOTMUX]], i32 1, !prof !13
>>>> > ; CHECK-NEXT: ret i32 [[OUTVAL]]
>>>> > ;
>>>> > block1:
>>>> > - br i1 %cmpb, label %block3, label %block2, !prof !0
>>>> > + br i1 %cmpa, label %block3, label %block2, !prof !13
>>>> >
>>>> > block2:
>>>> > - br i1 %cmpa, label %block3, label %exit, !prof !2
>>>> > + br i1 %cmpb, label %block3, label %exit, !prof !14
>>>> > +
>>>> > +block3:
>>>> > + %cowval = phi i32 [ 2, %block2 ], [ 0, %block1 ]
>>>> > + br label %exit
>>>> > +
>>>> > +exit:
>>>> > + %outval = phi i32 [ %cowval, %block3 ], [ 1, %block2 ]
>>>> > + ret i32 %outval
>>>> > +}
>>>> > +
>>>> > +; Swap the operands of the compares to verify that the weights
>>>> update correctly.
>>>> > +
>>>> > +define i32 @SimplifyCondBranchToCondBranchSwap(i1 %cmpa, i1 %cmpb) {
>>>> > +; CHECK-LABEL: @SimplifyCondBranchToCondBranchSwap(
>>>> > +; CHECK-NEXT: block1:
>>>> > +; CHECK-NEXT: [[CMPA_NOT:%.*]] = xor i1 %cmpa, true
>>>> > +; CHECK-NEXT: [[CMPB_NOT:%.*]] = xor i1 %cmpb, true
>>>> > +; CHECK-NEXT: [[BRMERGE:%.*]] = or i1 [[CMPA_NOT]], [[CMPB_NOT]]
>>>> > +; CHECK-NEXT: [[DOTMUX:%.*]] = select i1 [[CMPA_NOT]], i32 0, i32
>>>> 2, !prof !14
>>>> > +; CHECK-NEXT: [[OUTVAL:%.*]] = select i1 [[BRMERGE]], i32
>>>> [[DOTMUX]], i32 1, !prof !15
>>>> > +; CHECK-NEXT: ret i32 [[OUTVAL]]
>>>> > +;
>>>> > +block1:
>>>> > + br i1 %cmpa, label %block2, label %block3, !prof !13
>>>> > +
>>>> > +block2:
>>>> > + br i1 %cmpb, label %exit, label %block3, !prof !14
>>>> >
>>>> > block3:
>>>> > %cowval = phi i32 [ 2, %block2 ], [ 0, %block1 ]
>>>> > @@ -452,6 +478,8 @@ exit:
>>>> > !10 = !{!"branch_weights", i32 672646, i32 21604207}
>>>> > !11 = !{!"branch_weights", i32 6960, i32 21597248}
>>>> > !12 = !{!"these_are_not_the_branch_weights_you_are_looking_for", i32
>>>> 3, i32 5}
>>>> > +!13 = !{!"branch_weights", i32 2, i32 3}
>>>> > +!14 = !{!"branch_weights", i32 4, i32 7}
>>>> >
>>>> > ; CHECK: !0 = !{!"branch_weights", i32 5, i32 11}
>>>> > ; CHECK: !1 = !{!"branch_weights", i32 1, i32 5}
>>>> > @@ -467,5 +495,8 @@ exit:
>>>> > ;; treat the weight as an unsigned integer.
>>>> > ; CHECK: !10 = !{!"branch_weights", i32 112017436, i32 -735157296}
>>>> > ; CHECK: !11 = !{!"branch_weights", i32 3, i32 5}
>>>> > -; CHECK: !12 = !{!"branch_weights", i32 14, i32 10}
>>>> > +; CHECK: !12 = !{!"branch_weights", i32 22, i32 12}
>>>> > +; CHECK: !13 = !{!"branch_weights", i32 34, i32 21}
>>>> > +; CHECK: !14 = !{!"branch_weights", i32 33, i32 14}
>>>> > +; CHECK: !15 = !{!"branch_weights", i32 47, i32 8}
>>>> >
>>>> >
>>>> >
>>>> > _______________________________________________
>>>> > llvm-commits mailing list
>>>> > llvm-commits at lists.llvm.org
>>>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>
>>>
>>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160509/e7f769de/attachment.html>
More information about the llvm-commits
mailing list