Skip to content

Commit 0f0db6e

Browse files
OCHyamsOCHyams
committed
[KeyInstr] Merge atoms in DILocation::getMergedLocation (llvm#133480)
NFC for builds with LLVM_EXPERIMENTAL_KEY_INSTRUCTIONS=OFF (default). In an ideal world we would be able to track that the merged location is used in multiple source atoms. We can't do this though, so instead we arbitrarily but deterministically pick one. In cases where the InlinedAt field is unchanged we keep the atom with the lowest non-zero rank (highest precedence). If the ranks are equal we choose the smaller non-zero group number (arbitrary choice). In cases where the InlinedAt field is adjusted we generate a new atom group. Keeping the group wouldn't make sense (a source atom is identified by the group number and InlinedAt pair) but discarding the atom info could result in missed is_stmts. Add unittest in MetadataTest.cpp. RFC: https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668
1 parent 60aaa11 commit 0f0db6e

File tree

2 files changed

+205
-6
lines changed

2 files changed

+205
-6
lines changed

‎llvm/lib/IR/DebugInfoMetadata.cpp

Lines changed: 52 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -189,11 +189,15 @@ DILocation *DILocation::getMergedLocation(DILocation *LocA, DILocation *LocB) {
189189

190190
// Merge the two locations if possible, using the supplied
191191
// inlined-at location for the created location.
192-
auto MergeLocPair = [&C](const DILocation *L1, const DILocation *L2,
193-
DILocation *InlinedAt) -> DILocation * {
192+
auto *LocAIA = LocA->getInlinedAt();
193+
auto *LocBIA = LocB->getInlinedAt();
194+
auto MergeLocPair = [&C, LocAIA,
195+
LocBIA](const DILocation *L1, const DILocation *L2,
196+
DILocation *InlinedAt) -> DILocation * {
194197
if (L1 == L2)
195198
return DILocation::get(C, L1->getLine(), L1->getColumn(), L1->getScope(),
196-
InlinedAt);
199+
InlinedAt, L1->isImplicitCode(),
200+
L1->getAtomGroup(), L1->getAtomRank());
197201

198202
// If the locations originate from different subprograms we can't produce
199203
// a common location.
@@ -226,8 +230,47 @@ DILocation *DILocation::getMergedLocation(DILocation *LocA, DILocation *LocB) {
226230
bool SameCol = L1->getColumn() == L2->getColumn();
227231
unsigned Line = SameLine ? L1->getLine() : 0;
228232
unsigned Col = SameLine && SameCol ? L1->getColumn() : 0;
229-
230-
return DILocation::get(C, Line, Col, Scope, InlinedAt);
233+
bool IsImplicitCode = L1->isImplicitCode() && L2->isImplicitCode();
234+
235+
// Discard source location atom if the line becomes 0. And there's nothing
236+
// further to do if neither location has an atom number.
237+
if (!SameLine || !(L1->getAtomGroup() || L2->getAtomGroup()))
238+
return DILocation::get(C, Line, Col, Scope, InlinedAt, IsImplicitCode,
239+
/*AtomGroup*/ 0, /*AtomRank*/ 0);
240+
241+
uint64_t Group = 0;
242+
uint64_t Rank = 0;
243+
// If we're preserving the same matching inlined-at field we can
244+
// preserve the atom.
245+
if (LocBIA == LocAIA && InlinedAt == LocBIA) {
246+
// Deterministically keep the lowest non-zero ranking atom group
247+
// number.
248+
// FIXME: It would be nice if we could track that an instruction
249+
// belongs to two source atoms.
250+
bool UseL1Atom = [L1, L2]() {
251+
if (L1->getAtomRank() == L2->getAtomRank()) {
252+
// Arbitrarily choose the lowest non-zero group number.
253+
if (!L1->getAtomGroup() || !L2->getAtomGroup())
254+
return !L2->getAtomGroup();
255+
return L1->getAtomGroup() < L2->getAtomGroup();
256+
}
257+
// Choose the lowest non-zero rank.
258+
if (!L1->getAtomRank() || !L2->getAtomRank())
259+
return !L2->getAtomRank();
260+
return L1->getAtomRank() < L2->getAtomRank();
261+
}();
262+
Group = UseL1Atom ? L1->getAtomGroup() : L2->getAtomGroup();
263+
Rank = UseL1Atom ? L1->getAtomRank() : L2->getAtomRank();
264+
} else {
265+
// If either instruction is part of a source atom, reassign it a new
266+
// atom group. This essentially regresses to non-key-instructions
267+
// behaviour (now that it's the only instruction in its group it'll
268+
// probably get is_stmt applied).
269+
Group = C.incNextDILocationAtomGroup();
270+
Rank = 1;
271+
}
272+
return DILocation::get(C, Line, Col, Scope, InlinedAt, IsImplicitCode,
273+
Group, Rank);
231274
};
232275

233276
DILocation *Result = ARIt != ALocs.rend() ? (*ARIt)->getInlinedAt() : nullptr;
@@ -254,7 +297,10 @@ DILocation *DILocation::getMergedLocation(DILocation *LocA, DILocation *LocB) {
254297
// historically picked A's scope, and a nullptr inlined-at location, so that
255298
// behavior is mimicked here but I am not sure if this is always the correct
256299
// way to handle this.
257-
return DILocation::get(C, 0, 0, LocA->getScope(), nullptr);
300+
// Key Instructions: it's fine to drop atom group and rank here, as line 0
301+
// is a nonsensical is_stmt location.
302+
return DILocation::get(C, 0, 0, LocA->getScope(), nullptr, false,
303+
/*AtomGroup*/ 0, /*AtomRank*/ 0);
258304
}
259305

260306
std::optional<unsigned>

‎llvm/unittests/IR/MetadataTest.cpp

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1243,6 +1243,159 @@ TEST_F(DILocationTest, Merge) {
12431243
auto *M2 = DILocation::getMergedLocation(A2, B);
12441244
EXPECT_EQ(M1, M2);
12451245
}
1246+
#ifdef EXPERIMENTAL_KEY_INSTRUCTIONS
1247+
#define EXPECT_ATOM(Loc, Group, Rank) \
1248+
EXPECT_EQ(Group, M->getAtomGroup()); \
1249+
EXPECT_EQ(Rank, M->getAtomRank());
1250+
#else
1251+
#define EXPECT_ATOM(Loc, Group, Rank) \
1252+
EXPECT_EQ(0u, M->getAtomGroup()); \
1253+
EXPECT_EQ(0u, M->getAtomRank()); \
1254+
(void)Group; \
1255+
(void)Rank;
1256+
#endif
1257+
// Identical, including source atom numbers.
1258+
{
1259+
auto *A = DILocation::get(Context, 2, 7, N, nullptr, false, /*AtomGroup*/ 1,
1260+
/*AtomRank*/ 1);
1261+
auto *B = DILocation::get(Context, 2, 7, N, nullptr, false, /*AtomGroup*/ 1,
1262+
/*AtomRank*/ 1);
1263+
auto *M = DILocation::getMergedLocation(A, B);
1264+
EXPECT_ATOM(M, /*AtomGroup*/ 1u, 1u);
1265+
// DILocations are uniqued, so we can check equality by ptr.
1266+
EXPECT_EQ(M, DILocation::getMergedLocation(A, B));
1267+
}
1268+
1269+
// Identical but different atom ranks (same atom) - choose the lowest nonzero
1270+
// rank.
1271+
{
1272+
auto *A = DILocation::get(Context, 2, 7, N, nullptr, false, /*AtomGroup*/ 1,
1273+
/*AtomRank*/ 1);
1274+
auto *B = DILocation::get(Context, 2, 7, N, nullptr, false, /*AtomGroup*/ 1,
1275+
/*AtomRank*/ 2);
1276+
auto *M = DILocation::getMergedLocation(A, B);
1277+
EXPECT_ATOM(M, /*AtomGroup*/ 1u, /*AtomRank*/ 1u);
1278+
EXPECT_EQ(M, DILocation::getMergedLocation(B, A));
1279+
1280+
A = DILocation::get(Context, 2, 7, N, nullptr, false, /*AtomGroup*/ 1,
1281+
/*AtomRank*/ 0);
1282+
B = DILocation::get(Context, 2, 7, N, nullptr, false, /*AtomGroup*/ 1,
1283+
/*AtomRank*/ 2);
1284+
M = DILocation::getMergedLocation(A, B);
1285+
EXPECT_ATOM(M, /*AtomGroup*/ 1u, /*AtomRank*/ 2u);
1286+
EXPECT_EQ(M, DILocation::getMergedLocation(B, A));
1287+
}
1288+
1289+
// Identical but different atom ranks (different atom) - choose the lowest
1290+
// nonzero rank.
1291+
{
1292+
auto *A = DILocation::get(Context, 2, 7, N, nullptr, false, /*AtomGroup*/ 1,
1293+
/*AtomRank*/ 1);
1294+
auto *B = DILocation::get(Context, 2, 7, N, nullptr, false, /*AtomGroup*/ 2,
1295+
/*AtomRank*/ 2);
1296+
auto *M = DILocation::getMergedLocation(A, B);
1297+
EXPECT_ATOM(M, 1u, 1u);
1298+
EXPECT_EQ(M, DILocation::getMergedLocation(B, A));
1299+
1300+
A = DILocation::get(Context, 2, 7, N, nullptr, false, /*AtomGroup*/ 1,
1301+
/*AtomRank*/ 0);
1302+
B = DILocation::get(Context, 2, 7, N, nullptr, false, /*AtomGroup*/ 2,
1303+
/*AtomRank*/ 2);
1304+
M = DILocation::getMergedLocation(A, B);
1305+
EXPECT_ATOM(M, /*AtomGroup*/ 2u, /*AtomRank*/ 2u);
1306+
EXPECT_EQ(M, DILocation::getMergedLocation(B, A));
1307+
}
1308+
1309+
// Identical but equal atom rank (different atom) - choose the lowest non-zero
1310+
// group (arbitrary choice for deterministic behaviour).
1311+
{
1312+
auto *A = DILocation::get(Context, 2, 7, N, nullptr, false, /*AtomGroup*/ 1,
1313+
/*AtomRank*/ 1);
1314+
auto *B = DILocation::get(Context, 2, 7, N, nullptr, false, /*AtomGroup*/ 2,
1315+
/*AtomRank*/ 1);
1316+
auto *M = DILocation::getMergedLocation(A, B);
1317+
EXPECT_ATOM(M, 1u, 1u);
1318+
EXPECT_EQ(M, DILocation::getMergedLocation(B, A));
1319+
1320+
A = DILocation::get(Context, 2, 7, N, nullptr, false, /*AtomGroup*/ 0,
1321+
/*AtomRank*/ 1);
1322+
B = DILocation::get(Context, 2, 7, N, nullptr, false, /*AtomGroup*/ 2,
1323+
/*AtomRank*/ 1);
1324+
M = DILocation::getMergedLocation(A, B);
1325+
EXPECT_ATOM(M, /*AtomGroup*/ 2u, /*AtomRank*/ 1u);
1326+
EXPECT_EQ(M, DILocation::getMergedLocation(B, A));
1327+
}
1328+
1329+
// Completely different except same atom numbers. Zero out the atoms.
1330+
{
1331+
auto *I = DILocation::get(Context, 2, 7, N);
1332+
auto *A = DILocation::get(Context, 1, 6, S, I, false, /*AtomGroup*/ 1,
1333+
/*AtomRank*/ 1);
1334+
auto *B = DILocation::get(Context, 2, 7, getSubprogram(), nullptr, false,
1335+
/*AtomGroup*/ 1, /*AtomRank*/ 1);
1336+
auto *M = DILocation::getMergedLocation(A, B);
1337+
EXPECT_EQ(0u, M->getLine());
1338+
EXPECT_EQ(0u, M->getColumn());
1339+
EXPECT_TRUE(isa<DILocalScope>(M->getScope()));
1340+
EXPECT_EQ(S, M->getScope());
1341+
EXPECT_EQ(nullptr, M->getInlinedAt());
1342+
}
1343+
1344+
// Same inlined-at chain but different atoms. Choose the lowest
1345+
// non-zero group (arbitrary choice for deterministic behaviour).
1346+
{
1347+
auto *I = DILocation::get(Context, 1, 7, N);
1348+
auto *F = getSubprogram();
1349+
auto *A = DILocation::get(Context, 1, 1, F, I, false, /*AtomGroup*/ 1,
1350+
/*AtomRank*/ 2);
1351+
auto *B = DILocation::get(Context, 1, 1, F, I, false, /*AtomGroup*/ 2,
1352+
/*AtomRank*/ 2);
1353+
auto *M = DILocation::getMergedLocation(A, B);
1354+
EXPECT_ATOM(M, /*AtomGroup*/ 1u, /*AtomRank*/ 2u);
1355+
EXPECT_EQ(M, DILocation::getMergedLocation(B, A));
1356+
1357+
A = DILocation::get(Context, 1, 1, F, I, false, /*AtomGroup*/ 1,
1358+
/*AtomRank*/ 2);
1359+
B = DILocation::get(Context, 1, 1, F, I, false, /*AtomGroup*/ 2,
1360+
/*AtomRank*/ 0);
1361+
M = DILocation::getMergedLocation(A, B);
1362+
EXPECT_ATOM(M, /*AtomGroup*/ 1u, /*AtomRank*/ 2u);
1363+
EXPECT_EQ(M, DILocation::getMergedLocation(B, A));
1364+
}
1365+
1366+
// Partially equal inlined-at chain but different atoms. Generate a new atom
1367+
// group (if either have a group number). This configuration seems unlikely
1368+
// to occur as line numbers must match, but isn't impossible.
1369+
{
1370+
// Reset global counter to ensure EXPECT numbers line up.
1371+
Context.pImpl->NextAtomGroup = 1;
1372+
// x1 -> y2 -> z4
1373+
// y3 -> z4
1374+
auto *FX = getSubprogram();
1375+
auto *FY = getSubprogram();
1376+
auto *FZ = getSubprogram();
1377+
auto *Z4 = DILocation::get(Context, 1, 4, FZ);
1378+
auto *Y3IntoZ4 = DILocation::get(Context, 1, 3, FY, Z4, false,
1379+
/*AtomGroup*/ 1, /*AtomRank*/ 1);
1380+
auto *Y2IntoZ4 = DILocation::get(Context, 1, 2, FY, Z4);
1381+
auto *X1IntoY2 = DILocation::get(Context, 1, 1, FX, Y2IntoZ4);
1382+
auto *M = DILocation::getMergedLocation(X1IntoY2, Y3IntoZ4);
1383+
EXPECT_EQ(M->getScope(), FY);
1384+
EXPECT_EQ(M->getInlinedAt()->getScope(), FZ);
1385+
EXPECT_ATOM(M, /*AtomGroup*/ 2u, /*AtomRank*/ 1u);
1386+
1387+
// This swapped merge will produce a new atom group too.
1388+
M = DILocation::getMergedLocation(Y3IntoZ4, X1IntoY2);
1389+
1390+
// Same again, even if the atom numbers match.
1391+
auto *X1IntoY2SameAtom = DILocation::get(Context, 1, 1, FX, Y2IntoZ4, false,
1392+
/*AtomGroup*/ 1, /*AtomRank*/ 1);
1393+
M = DILocation::getMergedLocation(X1IntoY2SameAtom, Y3IntoZ4);
1394+
EXPECT_ATOM(M, /*AtomGroup*/ 4u, /*AtomRank*/ 1u);
1395+
M = DILocation::getMergedLocation(Y3IntoZ4, X1IntoY2SameAtom);
1396+
EXPECT_ATOM(M, /*AtomGroup*/ 5u, /*AtomRank*/ 1u);
1397+
}
1398+
#undef EXPECT_ATOM
12461399
}
12471400

12481401
TEST_F(DILocationTest, getDistinct) {

0 commit comments

Comments
 (0)