[llvm] r257795 - [SROA] Also insert a bit piece expression if only one piece is needed
James Molloy via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 15 03:51:03 PDT 2016
Hi,
Looking further at this, the issue stems from the type involved, which is
x86_fp80. This type has a n AllocSize of 128 bits but an "actual" size of
80 bits. Part of SROA is using the alloc size and another part is using the
non-alloc size which is why we get a size disparity.
I've put assertions in to this code that fire when DL.getTypeSizeInBits !=
DL.getTypeAllocSizeInBits, and those never fire in the test suite.
As this patch isn't sufficiently tested, I vote that we revert this patch.
Thoughts?
Cheers,
James
On Mon, 15 Aug 2016 at 10:52 James Molloy <james at jamesmolloy.co.uk> wrote:
>
> Hi Keno, Adrian,
>
> A recent change of mine to SimplifyCFG exposed a bug in this commit. This
> assert is firing:
>
> assert(Pieces.size() == 1 &&
> "partition is as large as original alloca");
>
> From the phab review it looks like this assert was added in late in the
> review process and my testing shows that this assert is never tested at all
> in the regression tests (changing to assert(0) doesn't cause any failures!)
>
> The error is shown here:
> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/23920/steps/annotate/logs/stdio
>
> It's easily reproducible with ToT clang simply compiling the
> file projects/compiler-rt/lib/builtins/powixf2.c *with debug info enabled
> (-g)*.
>
> I'd fix it myself but after staring for a while I can't work out if the
> the assert sense is wrong (the text "partition is as large as original
> alloca" seems like a failure mode to me, which would indicate that perhaps
> the test should have been "Pieces.size() != 1" ?) or if the assert is
> correct but there is a failure somewhere else.
>
> Could you please take a look?
>
> Cheers,
>
> James
>
> On Thu, 14 Jan 2016 at 20:10 Keno Fischer via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: kfischer
>> Date: Thu Jan 14 14:06:34 2016
>> New Revision: 257795
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=257795&view=rev
>> Log:
>> [SROA] Also insert a bit piece expression if only one piece is needed
>>
>> Summary: If SROA creates only one piece (e.g. because the other is not
>> needed),
>> it still needs to create a bit_piece expression if that bit piece is
>> smaller
>> than the original size of the alloca.
>>
>> Reviewers: aprantl
>>
>> Subscribers: llvm-commits
>>
>> Differential Revision: http://reviews.llvm.org/D16187
>>
>> Added:
>> llvm/trunk/test/Transforms/SROA/dbg-single-piece.ll
>> Modified:
>> llvm/trunk/lib/Transforms/Scalar/SROA.cpp
>>
>> Modified: llvm/trunk/lib/Transforms/Scalar/SROA.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/SROA.cpp?rev=257795&r1=257794&r2=257795&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Scalar/SROA.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Scalar/SROA.cpp Thu Jan 14 14:06:34 2016
>> @@ -4024,12 +4024,12 @@ bool SROA::splitAlloca(AllocaInst &AI, A
>> auto *Var = DbgDecl->getVariable();
>> auto *Expr = DbgDecl->getExpression();
>> DIBuilder DIB(*AI.getModule(), /*AllowUnresolved*/ false);
>> - bool IsSplit = Pieces.size() > 1;
>> + uint64_t AllocaSize = DL.getTypeSizeInBits(AI.getAllocatedType());
>> for (auto Piece : Pieces) {
>> // Create a piece expression describing the new partition or reuse
>> AI's
>> // expression if there is only one partition.
>> auto *PieceExpr = Expr;
>> - if (IsSplit || Expr->isBitPiece()) {
>> + if (Piece.Size < AllocaSize || Expr->isBitPiece()) {
>> // If this alloca is already a scalar replacement of a larger
>> aggregate,
>> // Piece.Offset describes the offset inside the scalar.
>> uint64_t Offset = Expr->isBitPiece() ? Expr->getBitPieceOffset()
>> : 0;
>> @@ -4043,6 +4043,9 @@ bool SROA::splitAlloca(AllocaInst &AI, A
>> Size = std::min(Size, AbsEnd - Start);
>> }
>> PieceExpr = DIB.createBitPieceExpression(Start, Size);
>> + } else {
>> + assert(Pieces.size() == 1 &&
>> + "partition is as large as original alloca");
>> }
>>
>> // Remove any existing dbg.declare intrinsic describing the same
>> alloca.
>>
>> Added: llvm/trunk/test/Transforms/SROA/dbg-single-piece.ll
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SROA/dbg-single-piece.ll?rev=257795&view=auto
>>
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/SROA/dbg-single-piece.ll (added)
>> +++ llvm/trunk/test/Transforms/SROA/dbg-single-piece.ll Thu Jan 14
>> 14:06:34 2016
>> @@ -0,0 +1,37 @@
>> +; RUN: opt -sroa %s -S | FileCheck %s
>> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
>> +
>> +%foo = type { [8 x i8], [8 x i8] }
>> +
>> +declare void @llvm.dbg.declare(metadata, metadata, metadata) #0
>> +define void
>> @_ZL18findInsertLocationPN4llvm17MachineBasicBlockENS_9SlotIndexERNS_13LiveIntervalsE()
>> {
>> +entry:
>> + %retval = alloca %foo, align 8
>> + call void @llvm.dbg.declare(metadata %foo* %retval, metadata !1,
>> metadata !7), !dbg !8
>> +; Checks that SROA still inserts a bit_piece expression, even if it
>> produces only one piece
>> +; (as long as that piece is smaller than the whole thing)
>> +; CHECK-NOT: call void @llvm.dbg.value
>> +; CHECK: call void @llvm.dbg.value(metadata %foo* undef, i64 0, metadata
>> !1, metadata ![[BIT_PIECE:[0-9]+]]), !dbg
>> +; CHECK-NOT: call void @llvm.dbg.value
>> +; CHECK: ![[BIT_PIECE]] = !DIExpression(DW_OP_bit_piece, 64, 64)
>> + %0 = bitcast %foo* %retval to i8*
>> + %1 = getelementptr inbounds i8, i8* %0, i64 8
>> + %2 = bitcast i8* %1 to %foo**
>> + store %foo* undef, %foo** %2, align 8
>> + ret void
>> +}
>> +
>> +attributes #0 = { nounwind readnone }
>> +
>> +!llvm.dbg.cu = !{}
>> +!llvm.module.flags = !{!0}
>> +
>> +!0 = !{i32 2, !"Debug Info Version", i32 3}
>> +!1 = !DILocalVariable(name: "I", scope: !2, file: !3, line: 947, type:
>> !4)
>> +!2 = distinct !DISubprogram(name: "findInsertLocation", linkageName:
>> "_ZL18findInsertLocationPN4llvm17MachineBasicBlockENS_9SlotIndexERNS_13LiveIntervalsE",
>> scope: !3, file: !3, line: 937, isLocal: true, isDefinition: true,
>> scopeLine: 938, flags: DIFlagPrototyped, isOptimized: true)
>> +!3 = !DIFile(filename: "none", directory: ".")
>> +!4 = !DICompositeType(tag: DW_TAG_class_type, name:
>> "bundle_iterator<llvm::MachineInstr,
>> llvm::ilist_iterator<llvm::MachineInstr> >", scope: !5, file: !3, line:
>> 163, size: 128, align: 64, elements: !6, templateParams: !6, identifier:
>> "_ZTSN4llvm17MachineBasicBlock15bundle_iteratorINS_12MachineInstrENS_14ilist_iteratorIS2_EEEE")
>> +!5 = distinct !DICompositeType(tag: DW_TAG_class_type, name:
>> "MachineBasicBlock", file: !3, line: 68, size: 1408, align: 64, identifier:
>> "_ZTSN4llvm17MachineBasicBlockE")
>> +!6 = !{}
>> +!7 = !DIExpression()
>> +!8 = !DILocation(line: 947, column: 35, scope: !2)
>>
>>
>> _______________________________________________
>> 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/20160815/2f570dd1/attachment-0001.html>
More information about the llvm-commits
mailing list