[SROA] Also insert a bit piece expression if only one piece is needed

[loladiro]

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.

[aprantl]

Thanks. LGTM with minor comments addressed.

lib/Transforms/Scalar/SROA.cpp

4032 ↗

(On Diff #44878)

Should we add an
} else assert(Pieces.size() == 1&& "partition is as large as original alloca")
?

test/Transforms/SROA/dbg-single-piece.ll

12 ↗

(On Diff #44878)

I think we should also check for the sroa'd alloca and verify that its size matches the size of the OP_bit_piece.

13 ↗

(On Diff #44878)

checking for the hardcoded !dbg !8 is just asking for trouble :-)

14 ↗

(On Diff #44878)

Comment?
; 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.

[loladiro]

lib/Transforms/Scalar/SROA.cpp

4032 ↗

(On Diff #44878)

Sounds reasonable.

test/Transforms/SROA/dbg-single-piece.ll

12 ↗

(On Diff #44878)

The problem is that it gets mem2reged so won't be in the final IR.

13 ↗

(On Diff #44878)

Yep, sorry.

14 ↗

(On Diff #44878)

I have a comment at the very top. Do you want me to move it here?

[aprantl]

test/Transforms/SROA/dbg-single-piece.ll

12 ↗

(On Diff #44878)

I see. Maybe add CHECK-NOT: dbg.value before and after to ensure we actually hit the case where there is only one alloca?

14 ↗

(On Diff #44878)

My bad! I didn't see it. That said, moving it closer to the CHECK might be a good idea.

llvm/trunk/lib/Transforms/Scalar/SROA.cpp

MaxPartitionsPerAlloca =
       std::max<unsigned>(NumPartitions, MaxPartitionsPerAlloca);
 
   // Migrate debug information from the old alloca to the new alloca(s)
   // and the individual partitions.
   if (DbgDeclareInst *DbgDecl = FindAllocaDbgDeclare(&AI)) {
     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;
         uint64_t Start = Offset + Piece.Offset;
         uint64_t Size = Piece.Size;
         if (Expr->isBitPiece()) {
           uint64_t AbsEnd = Expr->getBitPieceOffset() + Expr->getBitPieceSize();
           if (Start >= AbsEnd)
             // No need to describe a SROAed padding.
             continue;
           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.
       if (DbgDeclareInst *OldDDI = FindAllocaDbgDeclare(Piece.Alloca))
         OldDDI->eraseFromParent();
 
       DIB.insertDeclare(Piece.Alloca, Var, PieceExpr, DbgDecl->getDebugLoc(),
                         &AI);

llvm/trunk/test/Transforms/SROA/dbg-single-piece.ll

+; 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)