[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 10:23:23 PDT 2016
Oops, I recently added that code to CGP and forgot the '0' check. I'll make
the patch.
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.
On Mon, May 9, 2016 at 11:15 AM, Pete Cooper <peter_cooper at apple.com> wrote:
>
> On May 9, 2016, at 9:20 AM, Sanjay Patel <spatel at rotateright.com> wrote:
>
> 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.
>
> 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?
>
> 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}
>
> That then goes to CGP isFormingBranchFromSelectProfitable and crashes in
> here:
>
> if (SI->extractProfMetadata(TrueWeight, FalseWeight)) {
> uint64_t Max = std::max(TrueWeight, FalseWeight);
> uint64_t Sum = TrueWeight + FalseWeight;
> auto Probability = BranchProbability::getBranchProbability(Max, Sum);
> if (Probability > TLI->getPredictableBranchThreshold())
> return true;
> }
>
> If all we need to do is check for a 0 here then thats something I can
> write up a patch for.
>
> Thanks for all the help on this!
> Pete
>
>
> 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/f8d1a62e/attachment.html>
More information about the llvm-commits
mailing list