[KeyInstr][Clang] Do stmt atom by OCHyams · Pull Request #134644 · llvm/llvm-project

OCHyams · 2025-04-07T13:47:55Z

See test comment for possible future improvement.

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:
#130943

OCHyams · 2025-04-07T13:48:33Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

llvmbot · 2025-04-07T13:57:43Z

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

See test comment for possible future improvement.

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:
#130943

Full diff: https://github.com/llvm/llvm-project/pull/134644.diff

2 Files Affected:

(modified) clang/lib/CodeGen/CGStmt.cpp (+9-1)
(added) clang/test/KeyInstructions/do.c (+33)

diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index 7dd82d73d6b6a..e46cfb433ab89 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -1242,9 +1242,17 @@ void CodeGenFunction::EmitDoStmt(const DoStmt &S,
   // As long as the condition is true, iterate the loop.
   if (EmitBoolCondBranch) {
     uint64_t BackedgeCount = getProfileCount(S.getBody()) - ParentCount;
-    Builder.CreateCondBr(
+    auto *I = Builder.CreateCondBr(
         BoolCondVal, LoopBody, LoopExit.getBlock(),
         createProfileWeightsForLoop(S.getCond(), BackedgeCount));
+
+    // 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 (no jumping to the brace).
+    if (auto *I = dyn_cast<llvm::Instruction>(BoolCondVal))
+      addInstToNewSourceAtom(I, nullptr);
+    addInstToNewSourceAtom(I, nullptr);
   }
 
   LoopStack.pop();
diff --git a/clang/test/KeyInstructions/do.c b/clang/test/KeyInstructions/do.c
new file mode 100644
index 0000000000000..4b7a38cb35985
--- /dev/null
+++ b/clang/test/KeyInstructions/do.c
@@ -0,0 +1,33 @@
+// RUN: %clang -gkey-instructions -x c++ -std=c++17 %s -gmlt -S -emit-llvm -o - \
+// RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank
+
+// RUN: %clang -gkey-instructions -x c %s -gmlt -S -emit-llvm -o -  \
+// RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank
+
+// Perennial quesiton: should the `dec` be in its own source atom or not
+// (currently it is).
+
+// Another question - we've made the cmp and br separate source atoms for
+// now, to match existing behaviour in this case:
+// 1. do {
+// 2.   something();
+// 3. }
+// 4. while (--A);
+// Non key instruction behaviour is: 2, 4[, 3, 2, 4]+
+// The cond br is associated with the brace on line 3 and the cmp is line 4;
+// if they were in the same atom group we'd step just: 2, 3[, 2, 3]+
+// FIXME: We could arguably improve the behaviour by making them the same
+// group but having the cmp higher precedence, resulting in: 2, 4[, 2, 4]+.
+
+void a(int A) {
+// CHECK: %dec = add nsw i32 %0, -1, !dbg [[G1R2:!.*]]
+// CHECK: store i32 %dec, ptr %A.addr{{.*}}, !dbg [[G1R1:!.*]]
+// CHECK: %tobool = icmp ne i32 %dec, 0, !dbg [[G2R1:!.*]]
+// CHECK: br i1 %tobool, label %do.body, label %do.end, !dbg [[G3R1:!.*]], !llvm.loop
+    do { } while (--A);
+}
+
+// CHECK: [[G1R2]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 2)
+// CHECK: [[G1R1]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1)
+// CHECK: [[G2R1]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 1)
+// CHECK: [[G3R1]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 1)

jmorse

LGTM with some nits to address.

jmorse · 2025-05-22T23:20:30Z

        createProfileWeightsForLoop(S.getCond(), BackedgeCount));
+
+    // Key Instructions: Emit the condition and branch as separate atoms to
+    // match existing loop stepping behaviour. FIXME: We could have the branch


IMO "existing" should be expanded into what the stepping actually is, as this comment could unexpectedly go out of date.

jmorse · 2025-05-22T23:23:26Z

+    if (auto *I = dyn_cast<llvm::Instruction>(BoolCondVal))
+      addInstToNewSourceAtom(I, nullptr);


Please call the casted BoolCondVal something other than I to avoid the reader having to think about shadowed variables ._.

jmorse · 2025-05-22T23:25:52Z

+// RUN: %clang_cc1 -gkey-instructions -x c %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 `dec` be in its own source atom or not


Suggested change

// Perennial quesiton: should the `dec` be in its own source atom or not

// Perennial question: should the `dec` be in its own source atom or not

jmorse · 2025-05-22T23:28:58Z

+// FIXME: We could arguably improve the behaviour by making them the same
+// group but having the cmp higher precedence, resulting in: 2, 4[, 2, 4]+.


This sounds like the kind of FIXME that we should have a ticket for so that we don't forget it -- having enough information to decide stepping behaviours like this is great, and we should plan on improvements (once KIs have landed).

Done, in our internal issue tracker

See test comment for possible future improvement. 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: #130943

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Apr 7, 2025

OCHyams force-pushed the users/OCHyams/ki-clang-switch branch from eeee08d to 97617b6 Compare May 21, 2025 14:40

OCHyams force-pushed the users/OCHyams/ki-clang-do branch from 2ee4d45 to 58697b4 Compare May 21, 2025 14:45

jmorse approved these changes May 22, 2025

View reviewed changes

OCHyams force-pushed the users/OCHyams/ki-clang-switch branch from 97617b6 to 2e3a53b Compare May 23, 2025 09:17

OCHyams marked this pull request as draft May 23, 2025 10:25

OCHyams changed the base branch from users/OCHyams/ki-clang-switch to main May 23, 2025 10:26

OCHyams force-pushed the users/OCHyams/ki-clang-do branch from 58697b4 to 766c234 Compare May 23, 2025 10:26

OCHyams added 2 commits May 23, 2025 13:59

cc1

965a4fd

OCHyams force-pushed the users/OCHyams/ki-clang-do branch from 766c234 to 965a4fd Compare May 23, 2025 12:59

nits

7e14b73

OCHyams marked this pull request as ready for review May 23, 2025 13:30

OCHyams merged commit 189d5da into main May 23, 2025

OCHyams deleted the users/OCHyams/ki-clang-do branch May 23, 2025 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[KeyInstr][Clang] Do stmt atom#134644

[KeyInstr][Clang] Do stmt atom#134644
OCHyams merged 3 commits into
mainfrom
users/OCHyams/ki-clang-do

OCHyams commented Apr 7, 2025

Uh oh!

OCHyams commented Apr 7, 2025 •

edited

Loading

Uh oh!

llvmbot commented Apr 7, 2025 •

edited

Loading

Uh oh!

jmorse left a comment

Uh oh!

jmorse May 22, 2025

Uh oh!

OCHyams May 23, 2025

Uh oh!

jmorse May 22, 2025

Uh oh!

OCHyams May 23, 2025

Uh oh!

jmorse May 22, 2025

Uh oh!

OCHyams May 23, 2025

Uh oh!

jmorse May 22, 2025

Uh oh!

OCHyams May 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		if (auto *I = dyn_cast<llvm::Instruction>(BoolCondVal))
		addInstToNewSourceAtom(I, nullptr);

	// Perennial quesiton: should the `dec` be in its own source atom or not
	// Perennial question: should the `dec` be in its own source atom or not

		// FIXME: We could arguably improve the behaviour by making them the same
		// group but having the cmp higher precedence, resulting in: 2, 4[, 2, 4]+.

Uh oh!

Conversation

OCHyams commented Apr 7, 2025

Uh oh!

OCHyams commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmorse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

OCHyams commented Apr 7, 2025 •

edited

Loading

llvmbot commented Apr 7, 2025 •

edited

Loading