[KeyInstr][Clang] While stmt atom #134645

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

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:35Z

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

llvmbot · 2025-04-07T13:58:48Z

@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/134645.diff

2 Files Affected:

(modified) clang/lib/CodeGen/CGStmt.cpp (+8-1)
(added) clang/test/KeyInstructions/while.c (+34)

diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index e46cfb433ab89..d9fd406ad64ee 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -1130,7 +1130,14 @@ void CodeGenFunction::EmitWhileStmt(const WhileStmt &S,
     if (!Weights && CGM.getCodeGenOpts().OptimizationLevel)
       BoolCondVal = emitCondLikelihoodViaExpectIntrinsic(
           BoolCondVal, Stmt::getLikelihood(S.getBody()));
-    Builder.CreateCondBr(BoolCondVal, LoopBody, ExitBlock, Weights);
+    auto *I = Builder.CreateCondBr(BoolCondVal, LoopBody, 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. Explore this later.
+    if (auto *I = dyn_cast<llvm::Instruction>(BoolCondVal))
+      addInstToNewSourceAtom(I, nullptr);
+    addInstToNewSourceAtom(I, nullptr);
 
     if (ExitBlock != LoopExit.getBlock()) {
       EmitBlock(ExitBlock);
diff --git a/clang/test/KeyInstructions/while.c b/clang/test/KeyInstructions/while.c
new file mode 100644
index 0000000000000..f1bb91520315a
--- /dev/null
+++ b/clang/test/KeyInstructions/while.c
@@ -0,0 +1,34 @@
+// 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).
+
+// We've made the cmp and br separate source atoms for now, to match existing
+// behaviour in this case:
+// 1. while (
+// 2.   int i = --End
+// 3.   ) {
+// 4.   useValue(i);
+// 5. }
+// Without Key Instructions we go: 2, 1[, 4, 2, 1]+
+// Without separating cmp and br with Key Instructions we'd get:
+// 1[, 4, 1]+. If we made the cmp higher precedence than the
+// br and had them in the same group, we could get:
+// 2, [4, 2]+ which might be nicer. FIXME: do that later.
+
+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 %while.body, label %while.end, !dbg [[G3R1:!.*]]
+    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)

llvmbot · 2025-04-07T13:58:49Z

@llvm/pr-subscribers-clang-codegen

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/134645.diff

2 Files Affected:

(modified) clang/lib/CodeGen/CGStmt.cpp (+8-1)
(added) clang/test/KeyInstructions/while.c (+34)

diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index e46cfb433ab89..d9fd406ad64ee 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -1130,7 +1130,14 @@ void CodeGenFunction::EmitWhileStmt(const WhileStmt &S,
     if (!Weights && CGM.getCodeGenOpts().OptimizationLevel)
       BoolCondVal = emitCondLikelihoodViaExpectIntrinsic(
           BoolCondVal, Stmt::getLikelihood(S.getBody()));
-    Builder.CreateCondBr(BoolCondVal, LoopBody, ExitBlock, Weights);
+    auto *I = Builder.CreateCondBr(BoolCondVal, LoopBody, 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. Explore this later.
+    if (auto *I = dyn_cast<llvm::Instruction>(BoolCondVal))
+      addInstToNewSourceAtom(I, nullptr);
+    addInstToNewSourceAtom(I, nullptr);
 
     if (ExitBlock != LoopExit.getBlock()) {
       EmitBlock(ExitBlock);
diff --git a/clang/test/KeyInstructions/while.c b/clang/test/KeyInstructions/while.c
new file mode 100644
index 0000000000000..f1bb91520315a
--- /dev/null
+++ b/clang/test/KeyInstructions/while.c
@@ -0,0 +1,34 @@
+// 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).
+
+// We've made the cmp and br separate source atoms for now, to match existing
+// behaviour in this case:
+// 1. while (
+// 2.   int i = --End
+// 3.   ) {
+// 4.   useValue(i);
+// 5. }
+// Without Key Instructions we go: 2, 1[, 4, 2, 1]+
+// Without separating cmp and br with Key Instructions we'd get:
+// 1[, 4, 1]+. If we made the cmp higher precedence than the
+// br and had them in the same group, we could get:
+// 2, [4, 2]+ which might be nicer. FIXME: do that later.
+
+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 %while.body, label %while.end, !dbg [[G3R1:!.*]]
+    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 nits

jmorse · 2025-05-22T23:38:37Z

clang/lib/CodeGen/CGStmt.cpp

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


Rename inner I to avoid shadowing the outer one?

jmorse · 2025-05-22T23:40:17Z

clang/test/DebugInfo/KeyInstructions/while.c

+// 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:41:26Z

clang/test/DebugInfo/KeyInstructions/while.c

+// Without separating cmp and br with Key Instructions we'd get:
+// 1[, 4, 1]+. If we made the cmp higher precedence than the
+// br and had them in the same group, we could get:
+// 2, [4, 2]+ which might be nicer. FIXME: do that later.


I endorse this FIXME

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

This was referenced Apr 7, 2025

[KeyInstr][Clang] Switch stmt atom #134643

Open

[KeyInstr][Clang] Do stmt atom #134644

Merged

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-do branch from 2ee4d45 to 58697b4 Compare May 21, 2025 14:45

OCHyams force-pushed the users/OCHyams/ki-clang-while branch from 38f3eec to cdd6ff3 Compare May 21, 2025 14:47

jmorse approved these changes May 22, 2025

View reviewed changes

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

Base automatically changed from users/OCHyams/ki-clang-do to main May 23, 2025 13:31

OCHyams marked this pull request as draft May 23, 2025 13:31

OCHyams added 2 commits May 23, 2025 14:32

cc1

8b75d7f

OCHyams force-pushed the users/OCHyams/ki-clang-while branch from cdd6ff3 to 8b75d7f Compare May 23, 2025 13:32

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

nits

5b6d7a6

OCHyams merged commit 6bd3543 into main May 23, 2025
5 of 7 checks passed

OCHyams deleted the users/OCHyams/ki-clang-while branch May 23, 2025 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[KeyInstr][Clang] While stmt atom #134645

[KeyInstr][Clang] While stmt atom #134645

Uh oh!

OCHyams commented Apr 7, 2025

Uh oh!

OCHyams commented Apr 7, 2025 •

edited

Loading

Uh oh!

llvmbot commented Apr 7, 2025

Uh oh!

llvmbot commented Apr 7, 2025

Uh oh!

jmorse left a comment

Uh oh!

jmorse May 22, 2025

Uh oh!

jmorse May 22, 2025

Uh oh!

jmorse May 22, 2025

Uh oh!

Uh oh!

Uh oh!

		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

[KeyInstr][Clang] While stmt atom #134645

[KeyInstr][Clang] While stmt atom #134645

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

Uh oh!

llvmbot commented Apr 7, 2025

Uh oh!

jmorse left a comment

Choose a reason for hiding this comment

Uh oh!

jmorse May 22, 2025

Choose a reason for hiding this comment

Uh oh!

jmorse May 22, 2025

Choose a reason for hiding this comment

Uh oh!

jmorse May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

OCHyams commented Apr 7, 2025 •

edited

Loading