[llvm] 086635d - [Assignment Tracking][SROA] Fix fragment when slice size equals variable size
via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 6 07:33:57 PDT 2023
Author: OCHyams
Date: 2023-04-06T15:29:18+01:00
New Revision: 086635d6b9f74ec5f4b7cedb2aee94f07c3b8fde
URL: https://github.com/llvm/llvm-project/commit/086635d6b9f74ec5f4b7cedb2aee94f07c3b8fde
DIFF: https://github.com/llvm/llvm-project/commit/086635d6b9f74ec5f4b7cedb2aee94f07c3b8fde.diff
LOG: [Assignment Tracking][SROA] Fix fragment when slice size equals variable size
Correctly handle the case of splitting an alloca which backs contiguous
distinct variables, where a slice's size equals the size of a backed variable.
We need to ensure that we don't generate fragments expressions with fragments
of the same size as the variable as this is a verifier error.
Prior to this patch a fragment expression would be created in this
situation. e.g. splitting an alloca i64 with two adjacent 32-bit variables into
two 32-bit allocas, the new dbg.assign expressions would contain
(DW_OP_LLVM_fragment, 0, 32) and (DW_OP_LLVM_fragment, 32, 32) even though
those fragments cover each variable entirely.
Reviewed By: jmorse
Differential Revision: https://reviews.llvm.org/D147696
Added:
llvm/test/DebugInfo/Generic/assignment-tracking/sroa/var-sized-fragment.ll
Modified:
llvm/lib/Transforms/Scalar/SROA.cpp
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/Scalar/SROA.cpp b/llvm/lib/Transforms/Scalar/SROA.cpp
index 5eb7ae063a171..f7011672c8b5f 100644
--- a/llvm/lib/Transforms/Scalar/SROA.cpp
+++ b/llvm/lib/Transforms/Scalar/SROA.cpp
@@ -126,17 +126,18 @@ namespace {
/// Calculate the fragment of a variable to use when slicing a store
/// based on the slice dimensions, existing fragment, and base storage
/// fragment.
-/// Note that a returned value of std::nullopt indicates that there is
-/// no appropriate fragment available (rather than meaning use the whole
-/// variable, which is a common usage). Because the store is being sliced
-/// we always expect a fragment - there's never a case where the whole
-/// variable should be used.
-static std::optional<DIExpression::FragmentInfo>
-calculateFragment(uint64_t NewStorageSliceOffsetInBits,
+/// Results:
+/// UseFrag - Use Target as the new fragment.
+/// UseNoFrag - The new slice already covers the whole variable.
+/// Skip - The new alloca slice doesn't include this variable.
+enum FragCalcResult { UseFrag, UseNoFrag, Skip };
+static FragCalcResult
+calculateFragment(DILocalVariable *Variable,
+ uint64_t NewStorageSliceOffsetInBits,
uint64_t NewStorageSliceSizeInBits,
std::optional<DIExpression::FragmentInfo> StorageFragment,
- std::optional<DIExpression::FragmentInfo> CurrentFragment) {
- DIExpression::FragmentInfo Target;
+ std::optional<DIExpression::FragmentInfo> CurrentFragment,
+ DIExpression::FragmentInfo &Target) {
// If the base storage describes part of the variable apply the offset and
// the size constraint.
if (StorageFragment) {
@@ -149,20 +150,32 @@ calculateFragment(uint64_t NewStorageSliceOffsetInBits,
Target.OffsetInBits = NewStorageSliceOffsetInBits;
}
+ // If this slice extracts the entirety of an independent variable from a
+ // larger alloca, do not produce a fragment expression, as the variable is
+ // not fragmented.
+ if (!CurrentFragment) {
+ if (auto Size = Variable->getSizeInBits()) {
+ // Treat the current fragment as covering the whole variable.
+ CurrentFragment = DIExpression::FragmentInfo(*Size, 0);
+ if (Target == CurrentFragment)
+ return UseNoFrag;
+ }
+ }
+
// No additional work to do if there isn't a fragment already, or there is
// but it already exactly describes the new assignment.
if (!CurrentFragment || *CurrentFragment == Target)
- return Target;
+ return UseFrag;
// Reject the target fragment if it doesn't fit wholly within the current
// fragment. TODO: We could instead chop up the target to fit in the case of
// a partial overlap.
if (Target.startInBits() < CurrentFragment->startInBits() ||
Target.endInBits() > CurrentFragment->endInBits())
- return std::nullopt;
+ return Skip;
// Target fits within the current fragment, return it.
- return Target;
+ return UseFrag;
}
static DebugVariable getAggregateVariable(DbgVariableIntrinsic *DVI) {
@@ -237,25 +250,24 @@ static void migrateDebugInfo(AllocaInst *OldAlloca, bool IsSplit,
}
std::optional<DIExpression::FragmentInfo> CurrentFragment =
Expr->getFragmentInfo();
- std::optional<DIExpression::FragmentInfo> NewFragment =
- calculateFragment(OldAllocaOffsetInBits, SliceSizeInBits,
- BaseFragment, CurrentFragment);
- // Note that std::nullopt here means "skip this fragment" rather than
- // "there is no fragment / use the whole variable".
- if (!NewFragment)
- continue;
+ DIExpression::FragmentInfo NewFragment;
+ FragCalcResult Result = calculateFragment(
+ DbgAssign->getVariable(), OldAllocaOffsetInBits, SliceSizeInBits,
+ BaseFragment, CurrentFragment, NewFragment);
- if (!(NewFragment == CurrentFragment)) {
+ if (Result == Skip)
+ continue;
+ if (Result == UseFrag && !(NewFragment == CurrentFragment)) {
if (CurrentFragment) {
// Rewrite NewFragment to be relative to the existing one (this is
// what createFragmentExpression wants). CalculateFragment has
// already resolved the size for us. FIXME: Should it return the
// relative fragment too?
- NewFragment->OffsetInBits -= CurrentFragment->OffsetInBits;
+ NewFragment.OffsetInBits -= CurrentFragment->OffsetInBits;
}
// Add the new fragment info to the existing expression if possible.
if (auto E = DIExpression::createFragmentExpression(
- Expr, NewFragment->OffsetInBits, NewFragment->SizeInBits)) {
+ Expr, NewFragment.OffsetInBits, NewFragment.SizeInBits)) {
Expr = *E;
} else {
// Otherwise, add the new fragment info to an empty expression and
@@ -263,7 +275,7 @@ static void migrateDebugInfo(AllocaInst *OldAlloca, bool IsSplit,
// be computed with the new fragment.
Expr = *DIExpression::createFragmentExpression(
DIExpression::get(Expr->getContext(), std::nullopt),
- NewFragment->OffsetInBits, NewFragment->SizeInBits);
+ NewFragment.OffsetInBits, NewFragment.SizeInBits);
SetKillLocation = true;
}
}
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
new file mode 100644
index 0000000000000..59406b30d735a
--- /dev/null
+++ b/llvm/test/DebugInfo/Generic/assignment-tracking/sroa/var-sized-fragment.ll
@@ -0,0 +1,84 @@
+; RUN: opt -S %s -o - -passes=sroa | FileCheck %s
+
+;; SROA splits the alloca into two. Each slice already has a 32-bit variable
+;; associated with it (the structured binding variables). Check SROA doesn't
+;; mistakenly create 32 bit fragments for them.
+
+;; NOTE: The upper 32 bits are currently
+;; not tracked with assignment tracking (due to the dbg.declare containing an
+;; expression).
+;; The dbg intrinsics for the unnamed aggregate variable have been commented
+;; out to reduce test clutter.
+
+;; From C++ source:
+;; class two {public:int a; int b;}
+;; two get();
+;; int fun() {
+;; auto [a, b] = get();
+;; 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.0.0.extract.trunc, metadata ![[A:[0-9]+]], metadata !DIExpression())
+; CHECK: ![[A]] = !DILocalVariable(name: "a",
+; CHECK: ![[B]] = !DILocalVariable(name: "b",
+
+%class.two = type { i32, i32 }
+
+define dso_local noundef i32 @_Z3funv() #0 !dbg !10 {
+entry:
+ %0 = alloca %class.two, align 4, !DIAssignID !23
+ ;;call void @llvm.dbg.assign(metadata i1 undef, metadata !17, metadata !DIExpression(), metadata !23, metadata ptr %0, metadata !DIExpression()), !dbg !24
+ call void @llvm.dbg.assign(metadata i1 undef, metadata !15, metadata !DIExpression(), metadata !23, metadata ptr %0, metadata !DIExpression()), !dbg !24
+ call void @llvm.dbg.declare(metadata ptr %0, metadata !16, metadata !DIExpression(DW_OP_plus_uconst, 4)), !dbg !26
+ %call = call i64 @_Z3getv()
+ store i64 %call, ptr %0, align 4, !DIAssignID !28
+ ;;call void @llvm.dbg.assign(metadata i64 %call, metadata !17, metadata !DIExpression(), metadata !28, metadata ptr %0, metadata !DIExpression()), !dbg !24
+ call void @llvm.dbg.assign(metadata i64 %call, metadata !15, metadata !DIExpression(), metadata !28, metadata ptr %0, metadata !DIExpression()), !dbg !24
+ %a = getelementptr inbounds %class.two, ptr %0, i32 0, i32 0
+ %1 = load i32, ptr %a, align 4
+ ret i32 %1
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #2
+declare !dbg !38 i64 @_Z3getv() #3
+declare void @llvm.dbg.assign(metadata, metadata, metadata, metadata, metadata, metadata) #2
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5, !6, !7, !8}
+!llvm.ident = !{!9}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 17.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.cpp", directory: "h")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 8, !"PIC Level", i32 2}
+!6 = !{i32 7, !"PIE Level", i32 2}
+!7 = !{i32 7, !"uwtable", i32 2}
+!8 = !{i32 7, !"debug-info-assignment-tracking", i1 true}
+!9 = !{!"clang version 17.0.0"}
+!10 = distinct !DISubprogram(name: "fun", linkageName: "_Z3funv", scope: !1, file: !1, line: 3, type: !11, scopeLine: 3, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !14)
+!11 = !DISubroutineType(types: !12)
+!12 = !{!13}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!14 = !{!15, !16, !17}
+!15 = !DILocalVariable(name: "a", scope: !10, file: !1, line: 4, type: !13)
+!16 = !DILocalVariable(name: "b", scope: !10, file: !1, line: 4, type: !13)
+!17 = !DILocalVariable(scope: !10, file: !1, line: 4, type: !18)
+!18 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "two", file: !19, line: 1, size: 64, flags: DIFlagTypePassByValue, elements: !20, identifier: "_ZTS3two")
+!19 = !DIFile(filename: "./include.h", directory: "/")
+!20 = !{!21, !22}
+!21 = !DIDerivedType(tag: DW_TAG_member, name: "a", scope: !18, file: !19, line: 1, baseType: !13, size: 32, flags: DIFlagPublic)
+!22 = !DIDerivedType(tag: DW_TAG_member, name: "b", scope: !18, file: !19, line: 1, baseType: !13, size: 32, offset: 32, flags: DIFlagPublic)
+!23 = distinct !DIAssignID()
+!24 = !DILocation(line: 0, scope: !10)
+!26 = !DILocation(line: 4, column: 12, scope: !10)
+!28 = distinct !DIAssignID()
+!38 = !DISubprogram(name: "get", linkageName: "_Z3getv", scope: !1, file: !1, line: 2, type: !39, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !41)
+!39 = !DISubroutineType(types: !40)
+!40 = !{!18}
+!41 = !{}
More information about the llvm-commits
mailing list