[llvm] r279125 - [SLP] Initialize VectorizedValue when gathering
Matthew Simpson via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 19 14:54:56 PDT 2016
I'm also fine reverting the original patch in the meantime so the ubsan bot can go back to normal. Let me know if there's a preference here. Thanks!
-- Matt
-----Original Message-----
From: hwennborg at google.com [mailto:hwennborg at google.com] On Behalf Of Hans Wennborg
Sent: Friday, August 19, 2016 5:17 PM
To: Michael Kuperstein <mkuper at google.com>
Cc: Matthew Simpson <mssimpso at codeaurora.org>; LLVM Commits <llvm-commits at lists.llvm.org>
Subject: Re: [llvm] r279125 - [SLP] Initialize VectorizedValue when gathering
I probably won't treat it as a blocker, but if it's practical to pick up the fix (e.g. if we have to do a third release candidate anyway), I'll take it.
Thanks,
Hans
On Fri, Aug 19, 2016 at 2:08 PM, Michael Kuperstein <mkuper at google.com> wrote:
> Regardless of the fix, I don't think this should block 3.9 - the UB
> was not introduced by this commit, and is not a regression. The only
> thing that changed is that now we have an in-tree test that actually exercises the UB.
>
> Hans, what do you think?
>
>
> On Fri, Aug 19, 2016 at 12:04 PM, Matthew Simpson
> <mssimpso at codeaurora.org>
> wrote:
>>
>> I've submitted a fix for review here: https://reviews.llvm.org/D23723
>>
>>
>>
>> -- Matt
>>
>>
>>
>> From: Matthew Simpson [mailto:mssimpso at codeaurora.org]
>> Sent: Friday, August 19, 2016 1:30 PM
>> To: 'Michael Kuperstein' <mkuper at google.com>
>> Cc: 'LLVM Commits' <llvm-commits at lists.llvm.org>
>> Subject: RE: [llvm] r279125 - [SLP] Initialize VectorizedValue when
>> gathering
>>
>>
>>
>> No, we don’t. Performing the add in int64_t should work, though. I’ll
>> also probably need to update the test case since we’ll have a different cost.
>>
>>
>>
>> From: Michael Kuperstein [mailto:mkuper at google.com]
>> Sent: Friday, August 19, 2016 1:20 PM
>>
>>
>> To: Matthew Simpson <mssimpso at codeaurora.org>
>> Cc: LLVM Commits <llvm-commits at lists.llvm.org>
>> Subject: Re: [llvm] r279125 - [SLP] Initialize VectorizedValue when
>> gathering
>>
>>
>>
>> I don't think we have a SaturatingAdd for signed types.
>>
>>
>>
>> On Fri, Aug 19, 2016 at 10:05 AM, Matthew Simpson
>> <mssimpso at codeaurora.org> wrote:
>>
>> That's right. Can we just change this to a SaturatingAdd, then?
>>
>>
>>
>> -- Matt
>>
>>
>>
>> On Fri, Aug 19, 2016 at 12:58 PM, Michael Kuperstein via llvm-commits
>> <llvm-commits at lists.llvm.org> wrote:
>>
>> getTreeCost() can return INT_MAX, and tryToReduce() adds
>> getReductionCost() to that.
>>
>> Looks like the UB has been there for a while, the new test just
>> exposes it.
>>
>>
>>
>> On Fri, Aug 19, 2016 at 9:53 AM, Matthew Simpson via llvm-commits
>> <llvm-commits at lists.llvm.org> wrote:
>>
>> Sorry about that, I’ll take a look!
>>
>>
>>
>> -- Matt
>>
>>
>>
>> From: Kostya Serebryany [mailto:kcc at google.com]
>> Sent: Friday, August 19, 2016 12:43 PM
>> To: Matthew Simpson <mssimpso at codeaurora.org>
>> Cc: LLVM Commits <llvm-commits at lists.llvm.org>
>> Subject: Re: [llvm] r279125 - [SLP] Initialize VectorizedValue when
>> gathering
>>
>>
>>
>> I think this is causing the ubsan bot failure. please fix:
>>
>> lib/Transforms/Vectorize/SLPVectorizer.cpp:4177:34: runtime error:
>> signed integer overflow: 2147483647 + 11 cannot be represented in type 'int'
>>
>>
>> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/
>> 15655/steps/check-llvm%20ubsan/logs/stdio
>>
>>
>>
>> On Thu, Aug 18, 2016 at 12:50 PM, Matthew Simpson via llvm-commits
>> <llvm-commits at lists.llvm.org> wrote:
>>
>> Author: mssimpso
>> Date: Thu Aug 18 14:50:32 2016
>> New Revision: 279125
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=279125&view=rev
>> Log:
>> [SLP] Initialize VectorizedValue when gathering
>>
>> We abort building vectorizable trees in some cases (e.g., if the
>> maximum recursion depth is reached, if the region size is too large,
>> etc.). If this happens for a reduction, we can be left with a root
>> entry that needs to be gathered. For these cases, we need make sure
>> we actually set VectorizedValue to the resulting vector.
>>
>> This patch ensures we properly set VectorizedValue, and it also
>> ensures the insertelement sequence generated for the gathers is
>> inserted at the correct location.
>>
>> Reference: https://llvm.org/bugs/show_bug.cgi?id=28330
>> Differential Revison: https://reviews.llvm.org/D23410
>>
>> Added:
>> llvm/trunk/test/Transforms/SLPVectorizer/AArch64/gather-root.ll
>> Modified:
>> llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp
>>
>> Modified: llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectori
>> ze/SLPVectorizer.cpp?rev=279125&r1=279124&r2=279125&view=diff
>>
>> =====================================================================
>> =========
>> --- llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp Thu Aug 18
>> 14:50:32 2016
>> @@ -2153,11 +2153,61 @@ void BoUpSLP::reorderInputsAccordingToOp
>> }
>>
>> void BoUpSLP::setInsertPointAfterBundle(ArrayRef<Value *> VL) {
>> - Instruction *VL0 = cast<Instruction>(VL[0]);
>> - BasicBlock::iterator NextInst(VL0);
>> - ++NextInst;
>> - Builder.SetInsertPoint(VL0->getParent(), NextInst);
>> - Builder.SetCurrentDebugLocation(VL0->getDebugLoc());
>> +
>> + // Get the basic block this bundle is in. All instructions in the
>> bundle
>> + // should be in this block.
>> + auto *Front = cast<Instruction>(VL.front()); auto *BB =
>> + Front->getParent(); assert(all_of(make_range(VL.begin(),
>> + VL.end()), [&](Value *V) -> bool {
>> + return cast<Instruction>(V)->getParent() == BB; }));
>> +
>> + // The last instruction in the bundle in program order.
>> + Instruction *LastInst = nullptr;
>> +
>> + // Find the last instruction. The common case should be that BB
>> + has
>> been
>> + // scheduled, and the last instruction is VL.back(). So we start
>> + with // VL.back() and iterate over schedule data until we reach
>> + the end of
>> the
>> + // bundle. The end of the bundle is marked by null ScheduleData.
>> + if (BlocksSchedules.count(BB)) {
>> + auto *Bundle = BlocksSchedules[BB]->getScheduleData(VL.back());
>> + if (Bundle && Bundle->isPartOfBundle())
>> + for (; Bundle; Bundle = Bundle->NextInBundle)
>> + LastInst = Bundle->Inst;
>> + }
>> +
>> + // LastInst can still be null at this point if there's either not
>> + an
>> entry
>> + // for BB in BlocksSchedules or there's no ScheduleData available
>> + for // VL.back(). This can be the case if buildTree_rec aborts for
>> + various // reasons (e.g., the maximum recursion depth is reached,
>> + the maximum
>> region
>> + // size is reached, etc.). ScheduleData is initialized in the
>> scheduling
>> + // "dry-run".
>> + //
>> + // If this happens, we can still find the last instruction by
>> + brute
>> force. We
>> + // iterate forwards from Front (inclusive) until we either see all
>> + // instructions in the bundle or reach the end of the block. If
>> + Front
>> is the
>> + // last instruction in program order, LastInst will be set to
>> + Front,
>> and we
>> + // will visit all the remaining instructions in the block.
>> + //
>> + // One of the reasons we exit early from buildTree_rec is to place
>> + an
>> upper
>> + // bound on compile-time. Thus, taking an additional compile-time
>> + hit
>> here is
>> + // not ideal. However, this should be exceedingly rare since it
>> requires that
>> + // we both exit early from buildTree_rec and that the bundle be
>> out-of-order
>> + // (causing us to iterate all the way to the end of the block).
>> + if (!LastInst) {
>> + SmallPtrSet<Value *, 16> Bundle(VL.begin(), VL.end());
>> + for (auto &I : make_range(BasicBlock::iterator(Front), BB->end())) {
>> + if (Bundle.erase(&I))
>> + LastInst = &I;
>> + if (Bundle.empty())
>> + break;
>> + }
>> + }
>> +
>> + // Set the insertion point after the last instruction in the bundle.
>> Set the
>> + // debug location to Front.
>> + Builder.SetInsertPoint(BB, next(BasicBlock::iterator(LastInst)));
>> + Builder.SetCurrentDebugLocation(Front->getDebugLoc());
>> }
>>
>> Value *BoUpSLP::Gather(ArrayRef<Value *> VL, VectorType *Ty) { @@
>> -2235,7 +2285,9 @@ Value *BoUpSLP::vectorizeTree(TreeEntry
>>
>> if (E->NeedToGather) {
>> setInsertPointAfterBundle(E->Scalars);
>> - return Gather(E->Scalars, VecTy);
>> + auto *V = Gather(E->Scalars, VecTy);
>> + E->VectorizedValue = V;
>> + return V;
>> }
>>
>> unsigned Opcode = getSameOpcode(E->Scalars); @@ -2282,7 +2334,10
>> @@ Value *BoUpSLP::vectorizeTree(TreeEntry
>> E->VectorizedValue = V;
>> return V;
>> }
>> - return Gather(E->Scalars, VecTy);
>> + setInsertPointAfterBundle(E->Scalars);
>> + auto *V = Gather(E->Scalars, VecTy);
>> + E->VectorizedValue = V;
>> + return V;
>> }
>> case Instruction::ExtractValue: {
>> if (canReuseExtract(E->Scalars, Instruction::ExtractValue)) {
>> @@ -2294,7 +2349,10 @@ Value *BoUpSLP::vectorizeTree(TreeEntry
>> E->VectorizedValue = V;
>> return propagateMetadata(V, E->Scalars);
>> }
>> - return Gather(E->Scalars, VecTy);
>> + setInsertPointAfterBundle(E->Scalars);
>> + auto *V = Gather(E->Scalars, VecTy);
>> + E->VectorizedValue = V;
>> + return V;
>> }
>> case Instruction::ZExt:
>> case Instruction::SExt:
>>
>> Added:
>> llvm/trunk/test/Transforms/SLPVectorizer/AArch64/gather-root.ll
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SLPVec
>> torizer/AArch64/gather-root.ll?rev=279125&view=auto
>>
>> =====================================================================
>> =========
>> --- llvm/trunk/test/Transforms/SLPVectorizer/AArch64/gather-root.ll
>> (added)
>> +++ llvm/trunk/test/Transforms/SLPVectorizer/AArch64/gather-root.ll
>> +++ Thu
>> Aug 18 14:50:32 2016
>> @@ -0,0 +1,95 @@
>> +; RUN: opt < %s -slp-vectorizer -S | FileCheck %s
>> +--check-prefix=DEFAULT ; RUN: opt < %s -slp-recursion-max-depth=0
>> +-slp-vectorizer -S | FileCheck
>> %s --check-prefix=GATHER
>> +
>> +target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
>> +target triple = "aarch64--linux-gnu"
>> +
>> + at a = common global [80 x i8] zeroinitializer, align 16
>> +
>> +; DEFAULT-LABEL: @PR28330(
>> +; DEFAULT: %tmp17 = phi i32 [ %tmp34, %for.body ], [ 0, %entry ] ;
>> +DEFAULT: %tmp18 = phi i32 [ %tmp35, %for.body ], [ %n, %entry ] ;
>> +DEFAULT: %[[S0:.+]] = select <8 x i1> %1, <8 x i32> <i32 -720, i32
>> -720, i32 -720, i32 -720, i32 -720, i32 -720, i32 -720, i32 -720>, <8
>> x i32>
>> <i32 -80, i32 -80, i32 -80, i32 -80, i32 -80, i32 -80, i32 -80, i32
>> -80>
>> +; DEFAULT: %[[R0:.+]] = shufflevector <8 x i32> %[[S0]], <8 x i32>
>> +undef,
>> <8 x i32> <i32 4, i32 5, i32 6, i32 7, i32 undef, i32 undef, i32
>> undef, i32
>> undef>
>> +; DEFAULT: %[[R1:.+]] = add <8 x i32> %[[S0]], %[[R0]] ; DEFAULT:
>> +%[[R2:.+]] = shufflevector <8 x i32> %[[R1]], <8 x i32> undef,
>> <8 x i32> <i32 2, i32 3, i32 undef, i32 undef, i32 undef, i32 undef,
>> i32 undef, i32 undef>
>> +; DEFAULT: %[[R3:.+]] = add <8 x i32> %[[R1]], %[[R2]] ; DEFAULT:
>> +%[[R4:.+]] = shufflevector <8 x i32> %[[R3]], <8 x i32> undef,
>> <8 x i32> <i32 1, i32 undef, i32 undef, i32 undef, i32 undef, i32
>> undef, i32 undef, i32 undef>
>> +; DEFAULT: %[[R5:.+]] = add <8 x i32> %[[R3]], %[[R4]] ; DEFAULT:
>> +%[[R6:.+]] = extractelement <8 x i32> %[[R5]], i32 0 ; DEFAULT:
>> +%tmp34 = add i32 %[[R6]], %tmp17 ; ; GATHER-LABEL: @PR28330( ;
>> +GATHER: %tmp17 = phi i32 [ %tmp34, %for.body ], [ 0, %entry ] ;
>> +GATHER: %tmp18 = phi i32 [ %tmp35, %for.body ], [ %n, %entry ] ;
>> +GATHER: %tmp19 = select i1 %tmp1, i32 -720, i32 -80 ; GATHER: %tmp21
>> += select i1 %tmp3, i32 -720, i32 -80 ; GATHER: %tmp23 = select i1
>> +%tmp5, i32 -720, i32 -80 ; GATHER: %tmp25 = select i1 %tmp7, i32
>> +-720, i32 -80 ; GATHER: %tmp27 = select i1 %tmp9, i32 -720, i32 -80
>> +; GATHER: %tmp29 = select i1 %tmp11, i32 -720, i32 -80 ; GATHER:
>> +%tmp31 = select i1 %tmp13, i32 -720, i32 -80 ; GATHER: %tmp33 =
>> +select i1 %tmp15, i32 -720, i32 -80 ; GATHER: %[[I0:.+]] =
>> +insertelement <8 x i32> undef, i32 %tmp19, i32 0 ; GATHER:
>> +%[[I1:.+]] = insertelement <8 x i32> %[[I0]], i32 %tmp21, i32 1 ;
>> +GATHER: %[[I2:.+]] = insertelement <8 x i32> %[[I1]], i32 %tmp23,
>> +i32 2 ; GATHER: %[[I3:.+]] = insertelement <8 x i32> %[[I2]], i32
>> +%tmp25, i32 3 ; GATHER: %[[I4:.+]] = insertelement <8 x i32>
>> +%[[I3]], i32 %tmp27, i32 4 ; GATHER: %[[I5:.+]] = insertelement <8 x
>> +i32> %[[I4]], i32 %tmp29, i32 5 ; GATHER: %[[I6:.+]] = insertelement
>> +<8 x i32> %[[I5]], i32 %tmp31, i32 6 ; GATHER: %[[I7:.+]] =
>> +insertelement <8 x i32> %[[I6]], i32 %tmp33, i32 7 ; GATHER:
>> +%[[R0:.+]] = shufflevector <8 x i32> %[[I7]], <8 x i32> undef,
>> <8 x i32> <i32 4, i32 5, i32 6, i32 7, i32 undef, i32 undef, i32
>> undef, i32
>> undef>
>> +; GATHER: %[[R1:.+]] = add <8 x i32> %[[I7]], %[[R0]] ; GATHER:
>> +%[[R2:.+]] = shufflevector <8 x i32> %[[R1]], <8 x i32> undef,
>> <8 x i32> <i32 2, i32 3, i32 undef, i32 undef, i32 undef, i32 undef,
>> i32 undef, i32 undef>
>> +; GATHER: %[[R3:.+]] = add <8 x i32> %[[R1]], %[[R2]] ; GATHER:
>> +%[[R4:.+]] = shufflevector <8 x i32> %[[R3]], <8 x i32> undef,
>> <8 x i32> <i32 1, i32 undef, i32 undef, i32 undef, i32 undef, i32
>> undef, i32 undef, i32 undef>
>> +; GATHER: %[[R5:.+]] = add <8 x i32> %[[R3]], %[[R4]] ; GATHER:
>> +%[[R6:.+]] = extractelement <8 x i32> %[[R5]], i32 0 ; GATHER:
>> +%tmp34 = add i32 %[[R6]], %tmp17
>> +
>> +define void @PR28330(i32 %n) {
>> +entry:
>> + %tmp0 = load i8, i8* getelementptr inbounds ([80 x i8], [80 x i8]*
>> + at a,
>> i64 0, i64 1), align 1
>> + %tmp1 = icmp eq i8 %tmp0, 0
>> + %tmp2 = load i8, i8* getelementptr inbounds ([80 x i8], [80 x i8]*
>> + @a,
>> i64 0, i64 2), align 2
>> + %tmp3 = icmp eq i8 %tmp2, 0
>> + %tmp4 = load i8, i8* getelementptr inbounds ([80 x i8], [80 x i8]*
>> + @a,
>> i64 0, i64 3), align 1
>> + %tmp5 = icmp eq i8 %tmp4, 0
>> + %tmp6 = load i8, i8* getelementptr inbounds ([80 x i8], [80 x i8]*
>> + @a,
>> i64 0, i64 4), align 4
>> + %tmp7 = icmp eq i8 %tmp6, 0
>> + %tmp8 = load i8, i8* getelementptr inbounds ([80 x i8], [80 x i8]*
>> + @a,
>> i64 0, i64 5), align 1
>> + %tmp9 = icmp eq i8 %tmp8, 0
>> + %tmp10 = load i8, i8* getelementptr inbounds ([80 x i8], [80 x
>> + i8]* @a,
>> i64 0, i64 6), align 2
>> + %tmp11 = icmp eq i8 %tmp10, 0
>> + %tmp12 = load i8, i8* getelementptr inbounds ([80 x i8], [80 x
>> + i8]* @a,
>> i64 0, i64 7), align 1
>> + %tmp13 = icmp eq i8 %tmp12, 0
>> + %tmp14 = load i8, i8* getelementptr inbounds ([80 x i8], [80 x
>> + i8]* @a,
>> i64 0, i64 8), align 8
>> + %tmp15 = icmp eq i8 %tmp14, 0
>> + br label %for.body
>> +
>> +for.body:
>> + %tmp17 = phi i32 [ %tmp34, %for.body ], [ 0, %entry ]
>> + %tmp18 = phi i32 [ %tmp35, %for.body ], [ %n, %entry ]
>> + %tmp19 = select i1 %tmp1, i32 -720, i32 -80
>> + %tmp20 = add i32 %tmp17, %tmp19
>> + %tmp21 = select i1 %tmp3, i32 -720, i32 -80
>> + %tmp22 = add i32 %tmp20, %tmp21
>> + %tmp23 = select i1 %tmp5, i32 -720, i32 -80
>> + %tmp24 = add i32 %tmp22, %tmp23
>> + %tmp25 = select i1 %tmp7, i32 -720, i32 -80
>> + %tmp26 = add i32 %tmp24, %tmp25
>> + %tmp27 = select i1 %tmp9, i32 -720, i32 -80
>> + %tmp28 = add i32 %tmp26, %tmp27
>> + %tmp29 = select i1 %tmp11, i32 -720, i32 -80
>> + %tmp30 = add i32 %tmp28, %tmp29
>> + %tmp31 = select i1 %tmp13, i32 -720, i32 -80
>> + %tmp32 = add i32 %tmp30, %tmp31
>> + %tmp33 = select i1 %tmp15, i32 -720, i32 -80
>> + %tmp34 = add i32 %tmp32, %tmp33
>> + %tmp35 = add nsw i32 %tmp18, -1
>> + %tmp36 = icmp eq i32 %tmp35, 0
>> + br i1 %tmp36, label %for.end, label %for.body
>> +
>> +for.end:
>> + ret void
>> +}
>>
>>
>> _______________________________________________
>> 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
>>
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
>>
>>
>
>
More information about the llvm-commits
mailing list