[SROA] Fix incorrect offsets for structured binding variables #69007
Conversation
Addresses one part of llvm#61981: prior to this patch, upon splitting an alloca, SROA copied the dbg.declare onto the new allocas, updating the fragment info for each accordingly. This isn't enough; if there's an offset from the alloca in the DIExpression then it gets blindly copied to the new dbg.declare (i.e. the offset from the original alloca is copied over and applied to the new alloca). This patch fixes that by building on and using a utility funciton from Assignment Tracking called calculateFragmentIntersect to determine: a) The new fragment (existing code already did this), b) The new offset from the new alloca. Those parts are put together to create the new expression with a correct offset and fragment. Herein lies an key limitation of this approach too: calculateFragmentIntersect uses DIExression::extractIfOffset to grab an existing offset from the expression. extractIfOffset, and therefore calculateFragmentIntersect fails, if the expression contains anything other than an optional trivial offset and optional fragment. In that fail-state, debug-info for that variable fragment is dropped completely. That is to say, using this new approach, dbg.declares describing variable fragments that contain any non-offset non-fragment opcodes backed by allocas that are split are now dropped. If this is not an acceptable trade off, I suggest we simple undef/kill split alloca dbg.declares with offsets untill a more complete solution can be cooked up. That would probably involve interpreting the DIExpression and splicing the non-offset non-fragment part into the new expression. I'm not sure how difficult this is (it could be simple, but will probably be a little fiddly). Note: this fix doesn't address another similar issue. If two variables are stuffed into one alloca, then that alloca doesn't get split and mem2reg can promote it to a single (unsplit) value, the variable in the upper bits needs to have the offset in its DIExpression converted to a shift or a DW_OP_bit_piece (or otherwise describe that the variable starts at an offset into the value). I've called this out in the new test.
OCHyams
requested review from
jryans,
jmorse,
adrian-prantl,
SLTozer,
felipepiovezan,
scott-linder and
djtodoro
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-debuginfo Author: Orlando Cazalet-Hyams (OCHyams) ChangesAddresses one part of #61981: prior to this patch, upon splitting an alloca, SROA copied the dbg.declare onto the new allocas, updating the fragment info for each accordingly. This isn't enough; if there's an offset from the alloca in the DIExpression then it gets blindly copied to the new dbg.declare (i.e. the offset from the original alloca is copied over and applied to the new alloca). This patch fixes that by building on and using a utility function from Assignment Tracking called calculateFragmentIntersect to determine: a) The new fragment (existing code already did this), Those parts are put together to create the new expression with a correct offset and fragment. Herein lies an key limitation of this approach too: calculateFragmentIntersect uses DIExression::extractIfOffset to grab an existing offset from the expression. extractIfOffset, and therefore calculateFragmentIntersect fails, if the expression contains anything other than an optional trivial offset and optional fragment. In that fail-state, debug-info for that variable fragment is dropped completely. That is to say, using this new approach, dbg.declares describing variable fragments that contain any non-offset non-fragment opcodes backed by allocas that are split are now dropped. If this is not an acceptable trade off, I suggest we simple undef/kill split alloca dbg.declares with offsets untill a more complete solution can be cooked up. That would probably involve interpreting the DIExpression and splicing the non-offset non-fragment part into the new expression. I'm not sure how difficult this is (it could be simple, but will probably be a little fiddly). Note: this fix doesn't address another similar issue. If two variables are stuffed into one alloca, then that alloca doesn't get split and mem2reg can promote it to a single (unsplit) value, the variable in the upper bits needs to have the offset in its DIExpression converted to a shift or a DW_OP_bit_piece (or otherwise describe that the variable starts at an offset into the value). I've called this out in the new test. Depends on #69006 (I'm not sure how to create a patch stack over here on GitHub?). Patch is 30.84 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/69007.diff 7 Files Affected:
diff --git a/llvm/include/llvm/IR/DebugInfo.h b/llvm/include/llvm/IR/DebugInfo.h
index 92beebed8ad51df..7a821c0a5e574e2 100644
--- a/llvm/include/llvm/IR/DebugInfo.h
+++ b/llvm/include/llvm/IR/DebugInfo.h
@@ -226,7 +226,7 @@ void RAUW(DIAssignID *Old, DIAssignID *New);
/// Remove all Assignment Tracking related intrinsics and metadata from \p F.
void deleteAll(Function *F);
-/// Calculate the fragment of the variable in \p DAI covered
+/// Calculate the fragment of the variable in \p DVI covered
/// from (Dest + SliceOffsetInBits) to
/// to (Dest + SliceOffsetInBits + SliceSizeInBits)
///
@@ -235,10 +235,15 @@ void deleteAll(Function *F);
/// variable size) in DAI.
///
/// Result contains a zero-sized fragment if there's no intersect.
+/// \p DVI may be either a DbgDeclareInst or a DbgAssignIntrinsic.
+///
+/// \p NewExprOffsetInBits is set to the difference between the first bit of
+/// memory the fragment describes and the first bit of the slice.
bool calculateFragmentIntersect(
const DataLayout &DL, const Value *Dest, uint64_t SliceOffsetInBits,
- uint64_t SliceSizeInBits, const DbgAssignIntrinsic *DAI,
- std::optional<DIExpression::FragmentInfo> &Result);
+ uint64_t SliceSizeInBits, const DbgVariableIntrinsic *DVI,
+ std::optional<DIExpression::FragmentInfo> &Result,
+ uint64_t &NewExprOffsetInBits);
/// Helper struct for trackAssignments, below. We don't use the similar
/// DebugVariable class because trackAssignments doesn't (yet?) understand
diff --git a/llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp b/llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
index 5b56975587a7343..166f9f213afe91a 100644
--- a/llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
+++ b/llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
@@ -1981,12 +1981,15 @@ static AssignmentTrackingLowering::OverlapMap buildOverlapMapAndRecordDeclares(
for (DbgAssignIntrinsic *DAI : at::getAssignmentMarkers(Info->Base)) {
std::optional<DIExpression::FragmentInfo> FragInfo;
+ uint64_t NewExprOffsetInBits;
// Skip this assignment if the affected bits are outside of the
// variable fragment.
- if (!at::calculateFragmentIntersect(
- I.getModule()->getDataLayout(), Info->Base,
- Info->OffsetInBits, Info->SizeInBits, DAI, FragInfo) ||
- (FragInfo && FragInfo->SizeInBits == 0))
+ if (!at::calculateFragmentIntersect(I.getModule()->getDataLayout(),
+ Info->Base, Info->OffsetInBits,
+ Info->SizeInBits, DAI, FragInfo,
+ NewExprOffsetInBits) ||
+ (FragInfo && FragInfo->SizeInBits == 0) ||
+ NewExprOffsetInBits > 0)
continue;
// FragInfo from calculateFragmentIntersect is nullopt if the
diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp
index 48b5501c55ba47d..65edb5ebd8276f7 100644
--- a/llvm/lib/IR/DebugInfo.cpp
+++ b/llvm/lib/IR/DebugInfo.cpp
@@ -1782,8 +1782,34 @@ void at::deleteAll(Function *F) {
bool at::calculateFragmentIntersect(
const DataLayout &DL, const Value *Dest, uint64_t SliceOffsetInBits,
- uint64_t SliceSizeInBits, const DbgAssignIntrinsic *DAI,
- std::optional<DIExpression::FragmentInfo> &Result) {
+ uint64_t SliceSizeInBits, const DbgVariableIntrinsic *DVI,
+ std::optional<DIExpression::FragmentInfo> &Result,
+ uint64_t &NewExprOffsetInBits) {
+
+ // Only dbg.assign and dbg.declares are allowed because this function
+ // deals with memory locations. This isn't comprehensive because dbg.values
+ // are able to describe memory locations; support for dbg.values can be added
+ // if/when needed.
+ assert(isa<DbgAssignIntrinsic>(DVI) || isa<DbgDeclareInst>(DVI));
+
+ // There isn't a shared interface to get the "address" parts out of a
+ // dbg.declare and dbg.assign, so provide some wrappers now.
+ auto GetAddress = [DVI]() -> const Value * {
+ if (const auto *DAI = dyn_cast<DbgAssignIntrinsic>(DVI))
+ return DAI->getAddress();
+ return cast<DbgDeclareInst>(DVI)->getAddress();
+ };
+ auto IsKillLocation = [DVI]() -> bool {
+ if (const auto *DAI = dyn_cast<DbgAssignIntrinsic>(DVI))
+ return DAI->isKillAddress();
+ return cast<DbgDeclareInst>(DVI)->isKillLocation();
+ };
+ auto GetExpression = [DVI]() -> const DIExpression * {
+ if (const auto *DAI = dyn_cast<DbgAssignIntrinsic>(DVI))
+ return DAI->getAddressExpression();
+ return cast<DbgDeclareInst>(DVI)->getExpression();
+ };
+
// There are multiple offsets at play in this function, so let's break it
// down. Starting with how variables may be stored in allocas:
//
@@ -1874,10 +1900,10 @@ bool at::calculateFragmentIntersect(
// SliceOfVariable ∩ DAI_fragment = (offset: 144, size: 16)
// SliceOfVariable tells us the bits of the variable described by DAI that are
// affected by the DSE.
- if (DAI->isKillAddress())
+ if (IsKillLocation())
return false;
- DIExpression::FragmentInfo VarFrag = DAI->getFragmentOrEntireVariable();
+ DIExpression::FragmentInfo VarFrag = DVI->getFragmentOrEntireVariable();
if (VarFrag.SizeInBits == 0)
return false; // Variable size is unknown.
@@ -1885,12 +1911,12 @@ bool at::calculateFragmentIntersect(
// address-modifying expression.
int64_t PointerOffsetInBits;
{
- auto DestOffsetInBytes = DAI->getAddress()->getPointerOffsetFrom(Dest, DL);
+ auto DestOffsetInBytes = GetAddress()->getPointerOffsetFrom(Dest, DL);
if (!DestOffsetInBytes)
return false; // Can't calculate difference in addresses.
int64_t ExprOffsetInBytes;
- if (!DAI->getAddressExpression()->extractIfOffset(ExprOffsetInBytes))
+ if (!GetExpression()->extractIfOffset(ExprOffsetInBytes))
return false;
int64_t PointerOffsetInBytes = *DestOffsetInBytes + ExprOffsetInBytes;
@@ -1899,11 +1925,19 @@ bool at::calculateFragmentIntersect(
// Adjust the slice offset so that we go from describing the a slice
// of memory to a slice of the variable.
- int64_t NewOffsetInBits =
+ int64_t AdjustedSliceOffsetInBits =
SliceOffsetInBits + VarFrag.OffsetInBits - PointerOffsetInBits;
- if (NewOffsetInBits < 0)
- return false; // Fragment offsets can only be positive.
- DIExpression::FragmentInfo SliceOfVariable(SliceSizeInBits, NewOffsetInBits);
+ NewExprOffsetInBits = std::max(0l, -AdjustedSliceOffsetInBits);
+ AdjustedSliceOffsetInBits = std::max(0l, AdjustedSliceOffsetInBits);
+
+ // Check if the variable fragment sits outside this memory slice.
+ if (NewExprOffsetInBits >= SliceSizeInBits) {
+ Result = {0, 0};
+ return true;
+ }
+
+ DIExpression::FragmentInfo SliceOfVariable(
+ SliceSizeInBits - NewExprOffsetInBits, AdjustedSliceOffsetInBits);
// Intersect the variable slice with DAI's fragment to trim it down to size.
DIExpression::FragmentInfo TrimmedSliceOfVariable =
DIExpression::FragmentInfo::intersect(SliceOfVariable, VarFrag);
diff --git a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
index 0586ff191d94ed1..d5e7ffcf8060319 100644
--- a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
@@ -530,11 +530,12 @@ static void shortenAssignment(Instruction *Inst, Value *OriginalDest,
SmallVector<DbgAssignIntrinsic *> Linked(LinkedRange.begin(),
LinkedRange.end());
for (auto *DAI : Linked) {
+ uint64_t NewExprOffsetInBits;
std::optional<DIExpression::FragmentInfo> NewFragment;
if (!at::calculateFragmentIntersect(DL, OriginalDest, DeadSliceOffsetInBits,
- DeadSliceSizeInBits, DAI,
- NewFragment) ||
- !NewFragment) {
+ DeadSliceSizeInBits, DAI, NewFragment,
+ NewExprOffsetInBits) ||
+ !NewFragment || NewExprOffsetInBits > 0) {
// We couldn't calculate the intersecting fragment for some reason. Be
// cautious and unlink the whole assignment from the store.
DAI->setKillAddress();
diff --git a/llvm/lib/Transforms/Scalar/SROA.cpp b/llvm/lib/Transforms/Scalar/SROA.cpp
index 9200b83efa3a84a..8bb6419deeff2f9 100644
--- a/llvm/lib/Transforms/Scalar/SROA.cpp
+++ b/llvm/lib/Transforms/Scalar/SROA.cpp
@@ -4813,54 +4813,43 @@ bool SROAPass::splitAlloca(AllocaInst &AI, AllocaSlices &AS) {
for (auto *DbgAssign : at::getAssignmentMarkers(&AI))
DbgVariables.push_back(DbgAssign);
for (DbgVariableIntrinsic *DbgVariable : DbgVariables) {
- auto *Expr = DbgVariable->getExpression();
DIBuilder DIB(*AI.getModule(), /*AllowUnresolved*/ false);
- uint64_t AllocaSize =
- DL.getTypeSizeInBits(AI.getAllocatedType()).getFixedValue();
- for (auto Fragment : Fragments) {
- // Create a fragment expression describing the new partition or reuse AI's
- // expression if there is only one partition.
- auto *FragmentExpr = Expr;
- if (Fragment.Size < AllocaSize || Expr->isFragment()) {
- // If this alloca is already a scalar replacement of a larger aggregate,
- // Fragment.Offset describes the offset inside the scalar.
- auto ExprFragment = Expr->getFragmentInfo();
- uint64_t Offset = ExprFragment ? ExprFragment->OffsetInBits : 0;
- uint64_t Start = Offset + Fragment.Offset;
- uint64_t Size = Fragment.Size;
- if (ExprFragment) {
- uint64_t AbsEnd =
- ExprFragment->OffsetInBits + ExprFragment->SizeInBits;
- if (Start >= AbsEnd) {
- // No need to describe a SROAed padding.
- continue;
- }
- Size = std::min(Size, AbsEnd - Start);
- }
- // The new, smaller fragment is stenciled out from the old fragment.
- if (auto OrigFragment = FragmentExpr->getFragmentInfo()) {
- assert(Start >= OrigFragment->OffsetInBits &&
- "new fragment is outside of original fragment");
- Start -= OrigFragment->OffsetInBits;
- }
- // The alloca may be larger than the variable.
- auto VarSize = DbgVariable->getVariable()->getSizeInBits();
- if (VarSize) {
- if (Size > *VarSize)
- Size = *VarSize;
- if (Size == 0 || Start + Size > *VarSize)
- continue;
- }
-
- // Avoid creating a fragment expression that covers the entire variable.
- if (!VarSize || *VarSize != Size) {
- if (auto E =
- DIExpression::createFragmentExpression(Expr, Start, Size))
- FragmentExpr = *E;
- else
- continue;
- }
+ for (auto Fragment : Fragments) {
+ uint64_t OffestFromNewAllocaInBits;
+ std::optional<DIExpression::FragmentInfo> NewDbgFragment;
+
+ // Drop debug info for this variable fragment if we can't compute an
+ // intersect between it and the alloca slice.
+ if (!at::calculateFragmentIntersect(
+ DL, &AI, Fragment.Offset, Fragment.Size, DbgVariable,
+ NewDbgFragment, OffestFromNewAllocaInBits))
+ continue; // Do not migrate this fragment to this slice.
+
+ // Zero sized fragment indicates there's no intersect between the variable
+ // fragment and the alloca slice. Skip this slice for this variable
+ // fragment.
+ if (NewDbgFragment && !NewDbgFragment->SizeInBits)
+ continue; // Do not migrate this fragment to this slice.
+
+ // No fragment indicates DbgVariable's variable or fragment exactly
+ // overlaps the slice; copy its fragment (or nullopt if there isn't one).
+ if (!NewDbgFragment)
+ NewDbgFragment = DbgVariable->getFragment();
+
+ // calculateFragmentIntersect fails if DbgVariable's expression is not a
+ // trivial offset expression, meaning it must contains only an offset and
+ // fragment. Start from scratch; add the fragment and then the offset.
+ // If calculateFragmentIntersect gets smarter this needs updating
+ // (sroa-alloca-offset.ll should start failing) - we'd need to copy the
+ // other parts of the original expression.
+ DIExpression *NewExpr = DIExpression::get(AI.getContext(), {});
+ if (NewDbgFragment)
+ NewExpr = *DIExpression::createFragmentExpression(
+ NewExpr, NewDbgFragment->OffsetInBits, NewDbgFragment->SizeInBits);
+ if (OffestFromNewAllocaInBits > 0) {
+ int64_t OffsetInBytes = (OffestFromNewAllocaInBits + 7) / 8;
+ NewExpr = DIExpression::prepend(NewExpr, /*flags=*/0, OffsetInBytes);
}
// Remove any existing intrinsics on the new alloca describing
@@ -4884,14 +4873,14 @@ bool SROAPass::splitAlloca(AllocaInst &AI, AllocaSlices &AS) {
}
auto *NewAssign = DIB.insertDbgAssign(
Fragment.Alloca, DbgAssign->getValue(), DbgAssign->getVariable(),
- FragmentExpr, Fragment.Alloca, DbgAssign->getAddressExpression(),
+ NewExpr, Fragment.Alloca, DbgAssign->getAddressExpression(),
DbgAssign->getDebugLoc());
NewAssign->setDebugLoc(DbgAssign->getDebugLoc());
LLVM_DEBUG(dbgs() << "Created new assign intrinsic: " << *NewAssign
<< "\n");
} else {
- DIB.insertDeclare(Fragment.Alloca, DbgVariable->getVariable(),
- FragmentExpr, DbgVariable->getDebugLoc(), &AI);
+ DIB.insertDeclare(Fragment.Alloca, DbgVariable->getVariable(), NewExpr,
+ DbgVariable->getDebugLoc(), &AI);
}
}
}
diff --git a/llvm/test/DebugInfo/Generic/assignment-tracking/sroa/var-sized-fragment.ll b/llvm/test/DebugInfo/Generic/assignment-tracking/sroa/var-sized-fragment.ll
index 59406b30d735a83..b5a366e56defa37 100644
--- a/llvm/test/DebugInfo/Generic/assignment-tracking/sroa/var-sized-fragment.ll
+++ b/llvm/test/DebugInfo/Generic/assignment-tracking/sroa/var-sized-fragment.ll
@@ -18,10 +18,7 @@
;; return a;
;; }
-;; FIXME: Variable 'b' gets an incorrect location (value and expression) - see
-;; llvm.org/PR61981. This check just ensures that no fragment info is added to
-;; the dbg.value.
-; CHECK: dbg.value(metadata i32 %.sroa.0.0.extract.trunc, metadata ![[B:[0-9]+]], metadata !DIExpression(DW_OP_plus_uconst, 4))
+; CHECK: dbg.value(metadata i32 %.sroa.2.0.extract.trunc, metadata ![[B:[0-9]+]], metadata !DIExpression())
; CHECK: dbg.value(metadata i32 %.sroa.0.0.extract.trunc, metadata ![[A:[0-9]+]], metadata !DIExpression())
; CHECK: ![[A]] = !DILocalVariable(name: "a",
; CHECK: ![[B]] = !DILocalVariable(name: "b",
diff --git a/llvm/test/DebugInfo/Generic/sroa-alloca-offset.ll b/llvm/test/DebugInfo/Generic/sroa-alloca-offset.ll
new file mode 100644
index 000000000000000..3eaa165bee0eb98
--- /dev/null
+++ b/llvm/test/DebugInfo/Generic/sroa-alloca-offset.ll
@@ -0,0 +1,255 @@
+; RUN: opt %s -passes=sroa -S | FileCheck %s --check-prefixes=COMMON,OLD
+; RUN: opt %s -passes=declare-to-assign,sroa -S | FileCheck %s --check-prefixes=COMMON,NEW
+
+;; C++17 source:
+;; struct two { int a, b; } gt;
+;; int fun1() {
+;; auto [x, y] = gt;
+;; return x + y;
+;; }
+;;
+;; struct four { two a, b; } gf;
+;; int fun2() {
+;; auto [x, y] = gf;
+;; return x.a + y.b;
+;; }
+;; Plus some hand-written IR.
+;;
+;; Check that SROA understands how to split dbg.declares and dbg.assigns with
+;; offsets into their storge (e.g., the second variable in a structured binding
+;; is stored at an offset into the shared alloca).
+;;
+;; Additional notes:
+;; We expect the same dbg.value intrinsics to come out of SROA whether assignment
+;; tracking is enabled or not. However, the order of the debug intrinsics may
+;; differ, and assignment tracking replaces some dbg.declares with dbg.assigns.
+;;
+;; Structured bindings produce an artificial variable that covers the entire
+;; alloca. Although they add clutter to the test, they've been preserved in
+;; order to increase coverage. These use the placehold name 'A' in comments and
+;; checks.
+
+%struct.two = type { i32, i32 }
+%struct.four = type { %struct.two, %struct.two }
+
+@gt = dso_local global %struct.two zeroinitializer, align 4, !dbg !0
+@gf = dso_local global %struct.four zeroinitializer, align 4, !dbg !5
+
+
+; COMMON-LABEL: @_Z4fun1v
+; COMMON-NEXT: entry
+;; 32 bit variable x (!27): value a_reg.
+;;
+;; 32 bit variable y (!28): value b_reg.
+;;
+;; 64 bit variable A (!29) bits [0, 32): value a_reg.
+;; 64 bit variable A (!29) bits [32, 64): value b_reg.
+
+; OLD-NEXT: %[[a_reg:.*]] = load i32, ptr @gt
+; OLD-NEXT: llvm.dbg.value(metadata i32 %[[a_reg]], metadata ![[x0:[0-9]+]], metadata !DIExpression())
+; OLD-NEXT: llvm.dbg.value(metadata i32 %[[a_reg]], metadata ![[A0:[0-9]+]], metadata !DIExpression(DW_OP_LLVM_fragment, 0, 32))
+; OLD-NEXT: %[[b_reg:.*]] = load i32, ptr getelementptr inbounds (i8, ptr @gt, i64 4)
+; OLD-NEXT: llvm.dbg.value(metadata i32 %[[b_reg]], metadata ![[y0:[0-9]+]], metadata !DIExpression())
+; OLD-NEXT: llvm.dbg.value(metadata i32 %[[b_reg]], metadata ![[A0]], metadata !DIExpression(DW_OP_LLVM_fragment, 32, 32))
+
+; NEW-NEXT: %[[a_reg:.*]] = load i32, ptr @gt
+; NEW-NEXT: %[[b_reg:.*]] = load i32, ptr getelementptr inbounds (i8, ptr @gt, i64 4)
+; NEW-NEXT: llvm.dbg.value(metadata i32 %[[b_reg]], metadata ![[y0:[0-9]+]], metadata !DIExpression())
+; NEW-NEXT: llvm.dbg.value(metadata i32 %[[a_reg]], metadata ![[A0:[0-9]+]], metadata !DIExpression(DW_OP_LLVM_fragment, 0, 32))
+; NEW-NEXT: llvm.dbg.value(metadata i32 %[[b_reg]], metadata ![[A0]], metadata !DIExpression(DW_OP_LLVM_fragment, 32, 32))
+; NEW-NEXT: llvm.dbg.value(metadata i32 %[[a_reg]], metadata ![[x0:[0-9]+]], metadata !DIExpression())
+define dso_local noundef i32 @_Z4fun1v() #0 !dbg !23 {
+entry:
+ %0 = alloca %struct.two, align 4
+ call void @llvm.dbg.declare(metadata ptr %0, metadata !27, metadata !DIExpression()), !dbg !31
+ call void @llvm.dbg.declare(metadata ptr %0, metadata !28, metadata !DIExpression(DW_OP_plus_uconst, 4)), !dbg !31
+ call void @llvm.dbg.declare(metadata ptr %0, metadata !29, metadata !DIExpression()), !dbg !31
+ call void @llvm.memcpy.p0.p0.i64(ptr align 4 %0, ptr align 4 @gt, i64 8, i1 false), !dbg !31
+ %a = getelementptr inbounds %struct.two, ptr %0, i32 0, i32 0, !dbg !31
+ %1 = load i32, ptr %a, align 4, !dbg !31
+ %b = getelementptr inbounds %struct.two, ptr %0, i32 0, i32 1, !dbg !31
+ %2 = load i32, ptr %b, align 4, !dbg !31
+ %add = add nsw i32 %1, %2, !dbg !31
+ ret i32 %add, !dbg !31
+}
+
+; COMMON-LABEL: _Z4fun2v()
+; COMMON-NEXT: entry:
+;; 64 bit variable x (!50) bits [0, 32): value aa_reg.
+;; 64 bit variable x (!50) bits [32, 64): deref ab_ba_addr
+;;
+;; 64 bit variable y (!51) bits [0, 32): deref ab_ba_addr + 32
+;; 64 bit variable y (!51) bits [32, 64): value bb_reg.
+;;
+;; 128 bit variable A (!52) bits [0, 32): value aa_reg
+;; 128 bit variable A (!52) bits [32, 64): deref ab_ba_addr
+;; 128 bit variable A (!52) bits [96, 128): value bb_reg
+;;
+;; NOTE: This 8 byte alloca contains x.b (4 bytes) and y.a (4 bytes).
+; COMMON-NEXT: %[[ab_ba_addr:.*]] = alloca [8 x i8], align 4
+; OLD-NEXT: llvm.dbg.declare(metadata ptr %[[ab_ba_addr]], metadata ![[A1:[0-9]+]], metadata !DIExpression(DW_OP_LLVM_fragment, 32, 64))
+; OLD-NEXT: llvm.dbg.declare(metadata ptr %[[ab_ba_addr]], metadata ![[y1:[0-9]+]], metadata !DIExpression(DW_OP_plus_uconst, 4, DW_OP_LLVM_fragment, 0, 32))
+; OLD-NEXT: llvm.dbg.declare(metadata ptr %[[ab_ba_addr]], metadata ![[x1:[0-9]+]], metadata !DIExpression(DW_OP_LLVM_fragment, 32, 32))
+; OLD-NEXT: %[[aa_reg:.*]] = load i32, ptr @gf, align 4
+; OLD-NEXT: llvm.dbg.value(metadata i32 %[[aa_reg]], metadata ![[x1]], metadata !DIExpression(DW_OP_LLVM_fragment, 0, 32))
+; OLD-NEXT: llvm.dbg.value(metadata i32 %[[aa_reg]], metadata ![[A1]], metadata !DIExpression(DW_OP_LLVM_fragment, 0, 32))
+; OLD-NEXT: call void @llvm.memcpy{{.*}}(ptr align 4 %[[ab_ba_addr]], ptr align 4 getelementptr inbounds (i8, ptr @gf, i64 4), i64 8, i1 false)
+; OLD-NEXT: %[[bb_reg:.*]] = load i32, ptr getelementptr inbounds (i8, ptr @gf, i64 12), align 4
+; OLD-NEXT: llvm.dbg.value(metadata i32 %[[bb_reg]], metadata ![[y1]], metad...
[truncated]
|
| assert(isa<DbgAssignIntrinsic>(DVI) || isa<DbgDeclareInst>(DVI)); | ||
|
|
||
| // There isn't a shared interface to get the "address" parts out of a | ||
| // dbg.declare and dbg.assign, so provide some wrappers now. |
There was a problem hiding this comment.
Should this be a virtual function?
There was a problem hiding this comment.
I'm not 100% sure, I was on the fence about it so I erred on the side of minimising changes.
What would you suggest we do for the DbgValueInst overloads? I suppose that we could just return nullptr from getAddress (as there's no "address" part of a dbg.value).
I think it could get a little confusing for some of them, e.g. if we add getAddressExpression. getExpression != getAddressExpression for dbg.assign, but they are equal for a dbg.declare, and for a dbg.value getAddressExpression would either return nullptr or possibly confusingly also be an alias for getExpression. Then again, sufficient doc-comments should be able to explain the differences.
(I'm going to return to this patch a bit later, but thought I should get your thoughts on this while it's relatively fresh)
There was a problem hiding this comment.
Seems good in general although I have some test questions.
I think the only other concern is that there's some fiddly logic in splitAlloca to detect "too big" "too small" allocas, is there definitely test coverage for that that's ensuring we're getting it right?
|
|
||
| // Drop debug info for this variable fragment if we can't compute an | ||
| // intersect between it and the alloca slice. | ||
| if (!at::calculateFragmentIntersect( |
There was a problem hiding this comment.
Worth promoting this function out of the at:: namespace if it's being generalised so far?
| int64_t OffsetInBytes = (OffestFromNewAllocaInBits + 7) / 8; | ||
| NewExpr = DIExpression::prepend(NewExpr, /*flags=*/0, OffsetInBytes); |
There was a problem hiding this comment.
Worth asserting that these things are byte aligned (i.e. (Bits&7) == 0) to avoid unfortunate expectations in the future? I don't think any part of SROA etc is geared to cope with sub-byte positions anyway.
| ;; new alloca slices (i.e., check offset rewriting works correctly). Note that | ||
| ;; mem2reg incorrectly preserves the offest in the DIExpression a variable | ||
| ;; stuffed into the upper bits of a value (that is a bug), e.g. alloca+offset | ||
| ;; becomes vreg+offest. It should either convert the offest to a shift, or |
There was a problem hiding this comment.
Awkwardly, I think this wants something in all-caps with "FIXME" or similar, to draw attention to the fact that we test for incorrect behaviour. That avoids someone who's trying to work out what the correct behaviour is glazing over as they read the comments and not noticing the intention.
| ;; 16 bit variable h (!64): deref dead_64_128 | ||
| ; COMMON-NEXT: %[[dead_64_128:.*]] = alloca %struct.two | ||
| ; COMMON-NEXT: llvm.dbg.declare(metadata ptr %[[dead_64_128]], metadata ![[h:[0-9]+]], metadata !DIExpression()) |
There was a problem hiding this comment.
Sooo, if the offset is off the end of a slice, we point it back at the start? Would it be better / feasible to degrade to dbg.declare(undef...) instead?
|
Thanks for the reviews. While reviewing #69681 I noticed the original structured bindings test X includes a trivial example where DW_OP_deref is added to an expression in this scenario. Having the binding variables be references is enough: https://godbolt.org/z/E9EGnbEde That makes me less comfortable with this fix. I'll convert this PR to a draft for now; I think we should move forward with one of the options I mentioned in the summary (copied below). I won't be able to take a look at this immediately but will hopefully get back to it within a week or two.
|
|
Note that there's the DIExpression Optimiser in #69769 , which might make things simpler to manipulate (haven't looked at it yet) |
Just to double check something: under the current implementation prior to this patch, ignoring the bug fixed by this patch, we do the right thing in the presence of non-offset non-fragment opcodes? |
Yeah that's right (though it's slightly better than that - we only do the wrong thing when offsets are present. Expressions with fragments without offsets should be split up properly). |
Addresses one part of #61981: prior to this patch, upon splitting an alloca, SROA copied the dbg.declare onto the new allocas, updating the fragment info for each accordingly. This isn't enough; if there's an offset from the alloca in the DIExpression then it gets blindly copied to the new dbg.declare (i.e. the offset from the original alloca is copied over and applied to the new alloca).
This patch fixes that by building on and using a utility function from Assignment Tracking called calculateFragmentIntersect to determine:
a) The new fragment (existing code already did this),
b) The new offset from the new alloca.
Those parts are put together to create the new expression with a correct offset and fragment. Herein lies a key limitation of this approach too:
calculateFragmentIntersect uses DIExression::extractIfOffset to grab an existing offset from the expression. extractIfOffset, and therefore calculateFragmentIntersect, fails if the expression contains anything other than an optional trivial offset and optional fragment. In that fail-state, debug-info for that variable fragment is dropped completely.
That is to say, using this new approach, dbg.declares describing variable fragments that contain any non-offset non-fragment opcodes backed by allocas that are split are now dropped.
If this is not an acceptable trade off, I suggest we simply undef/kill split alloca dbg.declares with offsets until a more complete solution can be cooked up. That would probably involve interpreting the DIExpression and splicing the non-offset non-fragment part into the new expression. I'm not sure how difficult this is (it could be simple, but will probably be a little fiddly).
Note: this fix doesn't address another similar issue. If two variables are stuffed into one alloca, then that alloca doesn't get split and mem2reg can promote it to a single (unsplit) value, the variable in the upper bits needs to have the offset in its DIExpression converted to a shift or a DW_OP_bit_piece (or otherwise describe that the variable starts at an offset into the value). I've called this out in the new test.
Depends on #69006 (I'm not sure how to create a patch stack over here on GitHub?).