[clang] [KeyInstr][Clang] For range stmt atoms (PR #134647)
Orlando Cazalet-Hyams via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 3 07:09:43 PDT 2025
https://github.com/OCHyams updated https://github.com/llvm/llvm-project/pull/134647
>From 5c7ac16d5099b192f25f17ec58dbe89cb43a7bca Mon Sep 17 00:00:00 2001
From: Orlando Cazalet-Hyams <orlando.hyams at sony.com>
Date: Thu, 3 Apr 2025 19:46:01 +0100
Subject: [PATCH 1/6] [KeyInstr][Clang] For range stmt atoms
This patch is part of a stack that teaches Clang to generate Key Instructions
metadata for C and C++.
The Key Instructions project is introduced, including a "quick summary" section
at the top which adds context for this PR, here:
https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668
The feature is only functional in LLVM if LLVM is built with CMake flag
LLVM_EXPERIMENTAL_KEY_INSTRUCTIONs. Eventually that flag will be removed.
The Clang-side work is demoed here:
https://github.com/llvm/llvm-project/pull/130943
---
clang/lib/CodeGen/CGStmt.cpp | 13 +++-
.../DebugInfo/KeyInstructions/for-range.cpp | 72 +++++++++++++++++++
2 files changed, 84 insertions(+), 1 deletion(-)
create mode 100644 clang/test/DebugInfo/KeyInstructions/for-range.cpp
diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index 205a57cbab31a..9f9442b70f3b4 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -1483,7 +1483,14 @@ CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
if (!Weights && CGM.getCodeGenOpts().OptimizationLevel)
BoolCondVal = emitCondLikelihoodViaExpectIntrinsic(
BoolCondVal, Stmt::getLikelihood(S.getBody()));
- Builder.CreateCondBr(BoolCondVal, ForBody, ExitBlock, Weights);
+ auto *I = Builder.CreateCondBr(BoolCondVal, ForBody, ExitBlock, Weights);
+ // Key Instructions: Emit the condition and branch as separate atoms to
+ // match existing loop stepping behaviour. FIXME: We could have the branch as
+ // the backup location for the condition, which would probably be a better
+ // experience.
+ if (auto *I = dyn_cast<llvm::Instruction>(BoolCondVal))
+ addInstToNewSourceAtom(I, nullptr);
+ addInstToNewSourceAtom(I, nullptr);
if (ExitBlock != LoopExit.getBlock()) {
EmitBlock(ExitBlock);
@@ -1532,6 +1539,10 @@ CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
if (CGM.shouldEmitConvergenceTokens())
ConvergenceTokenStack.pop_back();
+
+ // We want the for closing brace to be step-able on to match existing
+ // behaviour.
+ addInstToNewSourceAtom(ForBody->getTerminator(), nullptr);
}
void CodeGenFunction::EmitReturnOfRValue(RValue RV, QualType Ty) {
diff --git a/clang/test/DebugInfo/KeyInstructions/for-range.cpp b/clang/test/DebugInfo/KeyInstructions/for-range.cpp
new file mode 100644
index 0000000000000..be71d9fbfb581
--- /dev/null
+++ b/clang/test/DebugInfo/KeyInstructions/for-range.cpp
@@ -0,0 +1,72 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 5
+// RUN: %clang -gkey-instructions %s -gmlt -S -emit-llvm -o - \
+// RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank
+
+// Perennial quesiton: should the inc be its own source atom or not
+// (currently it is).
+
+// FIXME: See do.c and while.c regarding cmp and cond br groups.
+
+// The stores in the setup (stores to __RANGE1, __BEGIN1, __END1) are all
+// marked as Key. Unclear whether that's desirable. Keep for now as that's
+// least risky.
+
+// Check the conditional branch (and the condition) in FOR_COND and
+// unconditional branch in FOR_BODY are Key Instructions.
+
+struct Range {
+ int *begin();
+ int *end();
+} r;
+
+// CHECK-LABEL: define dso_local void @_Z1av(
+// CHECK-SAME: ) #[[ATTR0:[0-9]+]] !dbg [[DBG10:![0-9]+]] {
+// CHECK-NEXT: [[ENTRY:.*:]]
+// CHECK-NEXT: [[__RANGE1:%.*]] = alloca ptr, align 8
+// CHECK-NEXT: [[__BEGIN1:%.*]] = alloca ptr, align 8
+// CHECK-NEXT: [[__END1:%.*]] = alloca ptr, align 8
+// CHECK-NEXT: [[I:%.*]] = alloca i32, align 4
+// CHECK-NEXT: store ptr @r, ptr [[__RANGE1]], align 8, !dbg [[DBG14:![0-9]+]]
+// CHECK-NEXT: [[CALL:%.*]] = call noundef ptr @_ZN5Range5beginEv(ptr noundef nonnull align 1 dereferenceable(1) @r), !dbg [[DBG15:![0-9]+]]
+// CHECK-NEXT: store ptr [[CALL]], ptr [[__BEGIN1]], align 8, !dbg [[DBG16:![0-9]+]]
+// CHECK-NEXT: [[CALL1:%.*]] = call noundef ptr @_ZN5Range3endEv(ptr noundef nonnull align 1 dereferenceable(1) @r), !dbg [[DBG17:![0-9]+]]
+// CHECK-NEXT: store ptr [[CALL1]], ptr [[__END1]], align 8, !dbg [[DBG18:![0-9]+]]
+// CHECK-NEXT: br label %[[FOR_COND:.*]], !dbg [[DBG19:![0-9]+]]
+// CHECK: [[FOR_COND]]:
+// CHECK-NEXT: [[TMP0:%.*]] = load ptr, ptr [[__BEGIN1]], align 8, !dbg [[DBG19]]
+// CHECK-NEXT: [[TMP1:%.*]] = load ptr, ptr [[__END1]], align 8, !dbg [[DBG19]]
+// CHECK-NEXT: [[CMP:%.*]] = icmp ne ptr [[TMP0]], [[TMP1]], !dbg [[DBG20:![0-9]+]]
+// CHECK-NEXT: br i1 [[CMP]], label %[[FOR_BODY:.*]], label %[[FOR_END:.*]], !dbg [[DBG21:![0-9]+]]
+// CHECK: [[FOR_BODY]]:
+// CHECK-NEXT: [[TMP2:%.*]] = load ptr, ptr [[__BEGIN1]], align 8, !dbg [[DBG19]]
+// CHECK-NEXT: [[TMP3:%.*]] = load i32, ptr [[TMP2]], align 4, !dbg [[DBG22:![0-9]+]]
+// CHECK-NEXT: store i32 [[TMP3]], ptr [[I]], align 4, !dbg [[DBG23:![0-9]+]]
+// CHECK-NEXT: br label %[[FOR_INC:.*]], !dbg [[DBG24:![0-9]+]]
+// CHECK: [[FOR_INC]]:
+// CHECK-NEXT: [[TMP4:%.*]] = load ptr, ptr [[__BEGIN1]], align 8, !dbg [[DBG19]]
+// CHECK-NEXT: [[INCDEC_PTR:%.*]] = getelementptr inbounds nuw i32, ptr [[TMP4]], i32 1, !dbg [[DBG25:![0-9]+]]
+// CHECK-NEXT: store ptr [[INCDEC_PTR]], ptr [[__BEGIN1]], align 8, !dbg [[DBG26:![0-9]+]]
+// CHECK-NEXT: br label %[[FOR_COND]], !dbg [[DBG19]], !llvm.loop
+// CHECK: [[FOR_END]]:
+// CHECK-NEXT: ret void, !dbg [[DBG30:![0-9]+]]
+//
+void a() {
+ for (int i: r)
+ ;
+}
+//.
+// CHECK: [[DBG14]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1)
+// CHECK: [[DBG15]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 2)
+// CHECK: [[DBG16]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 1)
+// CHECK: [[DBG17]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 2)
+// CHECK: [[DBG18]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 1)
+// CHECK: [[DBG19]] = !DILocation({{.*}})
+// CHECK: [[DBG20]] = !DILocation({{.*}}, atomGroup: 4, atomRank: 1)
+// CHECK: [[DBG21]] = !DILocation({{.*}}, atomGroup: 5, atomRank: 1)
+// CHECK: [[DBG22]] = !DILocation({{.*}}, atomGroup: 6, atomRank: 2)
+// CHECK: [[DBG23]] = !DILocation({{.*}}, atomGroup: 6, atomRank: 1)
+// CHECK: [[DBG24]] = !DILocation({{.*}} atomGroup: 8, atomRank: 1)
+// CHECK: [[DBG25]] = !DILocation({{.*}}, atomGroup: 7, atomRank: 2)
+// CHECK: [[DBG26]] = !DILocation({{.*}}, atomGroup: 7, atomRank: 1)
+// CHECK: [[DBG30]] = !DILocation({{.*}})
+//.
>From 254e88bf6fc8929857f1b3f58981ff2d08621a57 Mon Sep 17 00:00:00 2001
From: Orlando Cazalet-Hyams <orlando.hyams at sony.com>
Date: Wed, 21 May 2025 15:52:22 +0100
Subject: [PATCH 2/6] cc1 + nits
---
clang/lib/CodeGen/CGStmt.cpp | 2 +-
clang/test/DebugInfo/KeyInstructions/for-range.cpp | 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index 9f9442b70f3b4..e2e100532a88b 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -1541,7 +1541,7 @@ CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
ConvergenceTokenStack.pop_back();
// We want the for closing brace to be step-able on to match existing
- // behaviour.
+ // behaviour.
addInstToNewSourceAtom(ForBody->getTerminator(), nullptr);
}
diff --git a/clang/test/DebugInfo/KeyInstructions/for-range.cpp b/clang/test/DebugInfo/KeyInstructions/for-range.cpp
index be71d9fbfb581..3898942622e89 100644
--- a/clang/test/DebugInfo/KeyInstructions/for-range.cpp
+++ b/clang/test/DebugInfo/KeyInstructions/for-range.cpp
@@ -1,5 +1,4 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 5
-// RUN: %clang -gkey-instructions %s -gmlt -S -emit-llvm -o - \
+// RUN: %clang_cc1 -gkey-instructions %s -debug-info-kind=line-tables-only -emit-llvm -o - \
// RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank
// Perennial quesiton: should the inc be its own source atom or not
>From 29bd054a24a54b2860979b0e97f1af2bb3e73eab Mon Sep 17 00:00:00 2001
From: Orlando Cazalet-Hyams <orlando.hyams at sony.com>
Date: Tue, 3 Jun 2025 14:32:54 +0100
Subject: [PATCH 3/6] style nits
---
clang/lib/CodeGen/CGStmt.cpp | 4 ++--
clang/test/DebugInfo/KeyInstructions/for-range.cpp | 7 ++++---
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index e2e100532a88b..70295b58a80b7 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -1488,8 +1488,8 @@ CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
// match existing loop stepping behaviour. FIXME: We could have the branch as
// the backup location for the condition, which would probably be a better
// experience.
- if (auto *I = dyn_cast<llvm::Instruction>(BoolCondVal))
- addInstToNewSourceAtom(I, nullptr);
+ if (auto *CondI = dyn_cast<llvm::Instruction>(BoolCondVal))
+ addInstToNewSourceAtom(CondI, nullptr);
addInstToNewSourceAtom(I, nullptr);
if (ExitBlock != LoopExit.getBlock()) {
diff --git a/clang/test/DebugInfo/KeyInstructions/for-range.cpp b/clang/test/DebugInfo/KeyInstructions/for-range.cpp
index 3898942622e89..612641fe29cb7 100644
--- a/clang/test/DebugInfo/KeyInstructions/for-range.cpp
+++ b/clang/test/DebugInfo/KeyInstructions/for-range.cpp
@@ -1,14 +1,15 @@
-// RUN: %clang_cc1 -gkey-instructions %s -debug-info-kind=line-tables-only -emit-llvm -o - \
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -gkey-instructions %s -debug-info-kind=line-tables-only -emit-llvm -o - \
// RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank
-// Perennial quesiton: should the inc be its own source atom or not
+// Perennial question: should the inc be its own source atom or not
// (currently it is).
// FIXME: See do.c and while.c regarding cmp and cond br groups.
// The stores in the setup (stores to __RANGE1, __BEGIN1, __END1) are all
// marked as Key. Unclear whether that's desirable. Keep for now as that's
-// least risky.
+// least risky (at worst it introduces an unecessary step while debugging,
+// as opposed to potentially losing one we want).
// Check the conditional branch (and the condition) in FOR_COND and
// unconditional branch in FOR_BODY are Key Instructions.
>From f01b5ff93acb062de911dd5922731fa89f2160a0 Mon Sep 17 00:00:00 2001
From: Orlando Cazalet-Hyams <orlando.hyams at sony.com>
Date: Tue, 3 Jun 2025 14:50:56 +0100
Subject: [PATCH 4/6] fix body-with-cfg br atom, in line with for loop patch
---
clang/lib/CodeGen/CGStmt.cpp | 6 +++++-
.../DebugInfo/KeyInstructions/for-range.cpp | 21 ++++++++++++++++---
2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index 70295b58a80b7..79433769f0a28 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -1516,6 +1516,10 @@ CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
EmitStmt(S.getBody());
}
+ // The last block in the loop's body (which unconditionally branches to theAdd commentMore actions
+ // `inc` block if there is one).
+ auto *FinalBodyBB = Builder.GetInsertBlock();
+
EmitStopPoint(&S);
// If there is an increment, emit it next.
EmitBlock(Continue.getBlock());
@@ -1542,7 +1546,7 @@ CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
// We want the for closing brace to be step-able on to match existing
// behaviour.
- addInstToNewSourceAtom(ForBody->getTerminator(), nullptr);
+ addInstToNewSourceAtom(FinalBodyBB->getTerminator(), nullptr);
}
void CodeGenFunction::EmitReturnOfRValue(RValue RV, QualType Ty) {
diff --git a/clang/test/DebugInfo/KeyInstructions/for-range.cpp b/clang/test/DebugInfo/KeyInstructions/for-range.cpp
index 612641fe29cb7..3c6f9a69eca6a 100644
--- a/clang/test/DebugInfo/KeyInstructions/for-range.cpp
+++ b/clang/test/DebugInfo/KeyInstructions/for-range.cpp
@@ -1,5 +1,5 @@
// RUN: %clang_cc1 -triple x86_64-linux-gnu -gkey-instructions %s -debug-info-kind=line-tables-only -emit-llvm -o - \
-// RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank
+// RUN: | FileCheck %s
// Perennial question: should the inc be its own source atom or not
// (currently it is).
@@ -51,9 +51,22 @@ struct Range {
// CHECK-NEXT: ret void, !dbg [[DBG30:![0-9]+]]
//
void a() {
- for (int i: r)
- ;
+ for (int i: r)
+ ;
}
+
+// - Check the branch out of the body gets an atom group (and gets it correct
+// if there's ctrl-flow in the body).
+void b() {
+ for (int i: r) {
+ if (i)
+ ;
+// CHECK: entry:
+// CHECK: if.end:
+// CHECK-NEXT: br label %for.inc, !dbg [[b_br:!.*]]
+ }
+}
+
//.
// CHECK: [[DBG14]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1)
// CHECK: [[DBG15]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 2)
@@ -69,4 +82,6 @@ void a() {
// CHECK: [[DBG25]] = !DILocation({{.*}}, atomGroup: 7, atomRank: 2)
// CHECK: [[DBG26]] = !DILocation({{.*}}, atomGroup: 7, atomRank: 1)
// CHECK: [[DBG30]] = !DILocation({{.*}})
+//
+// CHECK: [[b_br]] = !DILocation({{.*}}, atomGroup: [[#]], atomRank: [[#]])
//.
>From dbe31d9ac821f0f5fe94ce8817e72215f1d46f37 Mon Sep 17 00:00:00 2001
From: Orlando Cazalet-Hyams <orlando.hyams at sony.com>
Date: Tue, 3 Jun 2025 15:09:07 +0100
Subject: [PATCH 5/6] Update clang/test/DebugInfo/KeyInstructions/for-range.cpp
Co-authored-by: Stephen Tozer <stephen.tozer at sony.com>
---
clang/test/DebugInfo/KeyInstructions/for-range.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/test/DebugInfo/KeyInstructions/for-range.cpp b/clang/test/DebugInfo/KeyInstructions/for-range.cpp
index 3c6f9a69eca6a..921060f3b2462 100644
--- a/clang/test/DebugInfo/KeyInstructions/for-range.cpp
+++ b/clang/test/DebugInfo/KeyInstructions/for-range.cpp
@@ -8,7 +8,7 @@
// The stores in the setup (stores to __RANGE1, __BEGIN1, __END1) are all
// marked as Key. Unclear whether that's desirable. Keep for now as that's
-// least risky (at worst it introduces an unecessary step while debugging,
+// least risky (at worst it introduces an unnecessary step while debugging,
// as opposed to potentially losing one we want).
// Check the conditional branch (and the condition) in FOR_COND and
>From 4cbd286e0ece5ae273fbce935de79f4fd585925b Mon Sep 17 00:00:00 2001
From: Orlando Cazalet-Hyams <orlando.hyams at sony.com>
Date: Tue, 3 Jun 2025 15:09:32 +0100
Subject: [PATCH 6/6] Update clang/lib/CodeGen/CGStmt.cpp
Co-authored-by: Stephen Tozer <stephen.tozer at sony.com>
---
clang/lib/CodeGen/CGStmt.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index 79433769f0a28..8739455481767 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -1516,7 +1516,7 @@ CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
EmitStmt(S.getBody());
}
- // The last block in the loop's body (which unconditionally branches to theAdd commentMore actions
+ // The last block in the loop's body (which unconditionally branches to the
// `inc` block if there is one).
auto *FinalBodyBB = Builder.GetInsertBlock();
More information about the cfe-commits
mailing list