[KeyInstr][Clang] Switch stmt atom #134643

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

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

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

llvmbot · 2025-04-07T13:55:42Z

@llvm/pr-subscribers-clang

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

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

2 Files Affected:

(modified) clang/lib/CodeGen/CGStmt.cpp (+2)
(added) clang/test/KeyInstructions/switch.c (+51)

diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index 3562b4ea22a24..7dd82d73d6b6a 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -2276,6 +2276,8 @@ void CodeGenFunction::EmitSwitchStmt(const SwitchStmt &S) {
   // failure.
   llvm::BasicBlock *DefaultBlock = createBasicBlock("sw.default");
   SwitchInsn = Builder.CreateSwitch(CondV, DefaultBlock);
+  addInstToNewSourceAtom(SwitchInsn, CondV);
+
   if (HLSLControlFlowAttr != HLSLControlFlowHintAttr::SpellingNotCalculated) {
     llvm::MDBuilder MDHelper(CGM.getLLVMContext());
     llvm::ConstantInt *BranchHintConstant =
diff --git a/clang/test/KeyInstructions/switch.c b/clang/test/KeyInstructions/switch.c
new file mode 100644
index 0000000000000..cff6b834106e9
--- /dev/null
+++ b/clang/test/KeyInstructions/switch.c
@@ -0,0 +1,51 @@
+// 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 --check-prefixes=CHECK,CHECK-CXX
+
+// RUN: %clang -gkey-instructions -x c %s -gmlt -S -emit-llvm -o -  \
+// RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank
+
+int g;
+void a(int A, int B) {
+// CHECK: entry:
+// The load gets associated with the branch rather than the store.
+// FIXME: Is that the best thing to do?
+// CHECK: %0 = load i32, ptr %A.addr{{.*}}, !dbg [[G2R2:!.*]]
+// CHECK: store i32 %0, ptr @g{{.*}}, !dbg [[G1R1:!.*]]
+// CHECK: switch i32 %0, label %{{.*}} [
+// CHECK:   i32 0, label %sw.bb
+// CHECK:   i32 1, label %sw.bb1
+// CHECK: ], !dbg [[G2R1:!.*]]
+    switch ((g = A)) {
+    case 0: break;
+    case 1: {
+// CHECK: sw.bb1:
+// CHECK: %1 = load i32, ptr %B.addr{{.*}}, !dbg [[G3R2:!.*]]
+// CHECK: switch i32 %1, label %{{.*}} [
+// CHECK:   i32 0, label %sw.bb2
+// CHECK: ], !dbg [[G3R1:!.*]]
+    switch ((B)) {
+        case 0: {
+// Test that assignments in constant-folded switches don't go missing.
+// CHECK-CXX: sw.bb2:
+// CHECK-CXX: store i32 1, ptr %C{{.*}}, !dbg [[G4R1:!.*]]
+#ifdef __cplusplus
+            switch (const int C = 1; C) {
+                case 0: break;
+                case 1: break;
+                default: break;
+            }
+#endif
+        } break;
+        default: break;
+    }
+    } break;
+    default: break;
+    }
+}
+
+// CHECK: [[G2R2]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 2)
+// CHECK: [[G1R1]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1)
+// CHECK: [[G2R1]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 1)
+// CHECK: [[G3R2]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 2)
+// CHECK: [[G3R1]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 1)
+// CHECK-CXX: [[G4R1]] = !DILocation({{.*}}, atomGroup: 4, atomRank: 1)

llvmbot · 2025-04-07T13:55:42Z

@llvm/pr-subscribers-clang-codegen

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

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

2 Files Affected:

(modified) clang/lib/CodeGen/CGStmt.cpp (+2)
(added) clang/test/KeyInstructions/switch.c (+51)

diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index 3562b4ea22a24..7dd82d73d6b6a 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -2276,6 +2276,8 @@ void CodeGenFunction::EmitSwitchStmt(const SwitchStmt &S) {
   // failure.
   llvm::BasicBlock *DefaultBlock = createBasicBlock("sw.default");
   SwitchInsn = Builder.CreateSwitch(CondV, DefaultBlock);
+  addInstToNewSourceAtom(SwitchInsn, CondV);
+
   if (HLSLControlFlowAttr != HLSLControlFlowHintAttr::SpellingNotCalculated) {
     llvm::MDBuilder MDHelper(CGM.getLLVMContext());
     llvm::ConstantInt *BranchHintConstant =
diff --git a/clang/test/KeyInstructions/switch.c b/clang/test/KeyInstructions/switch.c
new file mode 100644
index 0000000000000..cff6b834106e9
--- /dev/null
+++ b/clang/test/KeyInstructions/switch.c
@@ -0,0 +1,51 @@
+// 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 --check-prefixes=CHECK,CHECK-CXX
+
+// RUN: %clang -gkey-instructions -x c %s -gmlt -S -emit-llvm -o -  \
+// RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank
+
+int g;
+void a(int A, int B) {
+// CHECK: entry:
+// The load gets associated with the branch rather than the store.
+// FIXME: Is that the best thing to do?
+// CHECK: %0 = load i32, ptr %A.addr{{.*}}, !dbg [[G2R2:!.*]]
+// CHECK: store i32 %0, ptr @g{{.*}}, !dbg [[G1R1:!.*]]
+// CHECK: switch i32 %0, label %{{.*}} [
+// CHECK:   i32 0, label %sw.bb
+// CHECK:   i32 1, label %sw.bb1
+// CHECK: ], !dbg [[G2R1:!.*]]
+    switch ((g = A)) {
+    case 0: break;
+    case 1: {
+// CHECK: sw.bb1:
+// CHECK: %1 = load i32, ptr %B.addr{{.*}}, !dbg [[G3R2:!.*]]
+// CHECK: switch i32 %1, label %{{.*}} [
+// CHECK:   i32 0, label %sw.bb2
+// CHECK: ], !dbg [[G3R1:!.*]]
+    switch ((B)) {
+        case 0: {
+// Test that assignments in constant-folded switches don't go missing.
+// CHECK-CXX: sw.bb2:
+// CHECK-CXX: store i32 1, ptr %C{{.*}}, !dbg [[G4R1:!.*]]
+#ifdef __cplusplus
+            switch (const int C = 1; C) {
+                case 0: break;
+                case 1: break;
+                default: break;
+            }
+#endif
+        } break;
+        default: break;
+    }
+    } break;
+    default: break;
+    }
+}
+
+// CHECK: [[G2R2]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 2)
+// CHECK: [[G1R1]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1)
+// CHECK: [[G2R1]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 1)
+// CHECK: [[G3R2]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 2)
+// CHECK: [[G3R1]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 1)
+// CHECK-CXX: [[G4R1]] = !DILocation({{.*}}, atomGroup: 4, atomRank: 1)

jmorse

LGTM with some test nits, please do disagree with those

jmorse · 2025-05-22T23:03:10Z

clang/test/DebugInfo/KeyInstructions/switch.c

+void a(int A, int B) {
+// CHECK: entry:
+// The load gets associated with the branch rather than the store.
+// FIXME: Is that the best thing to do?


Interesting -- I wonder what happens when various bits get optimised away:

The switch probably can't be constant-propagated or otherwise affected without also messing with the load?

The store could be DSE'd quite easily

I feel the least desirable outcome is one where the switch disappears and the load gets is_stmt, thus leading to the load and store being steps, on the same line. And that seems unlikely to occur given that if the switch can be optimised, then the load has already been optimised.

A long way of saying, this is probably alright.

Consider also that we could have:

switch ((g = A)) {

If the store gets DSE'd and switch stays put, and we have the current setup (load in switch's atom group), then we don't step on ((g = A)). The store and load are line y and switch is line x. So non-key-instructions behaviour in that case is step y, x whereas with this patch it would be just x. If we associated the load with the store instead of the switch it'd be y, x (step on the load then the switch/br).

That's a long way of saying... maybe we should switch it round? I can't remember how tricky it would be. I think at worst it involves adding another param to addInstToNewSourceAtom to avoid overriding existing groups (atm it won't override an existing group if the new rank designation is lower priority.

We could make that <= instead too... that would impact the other dual-membership setups which we need to check all make sense after the change.

So - any objection to me adding this to the TODO list instead, to submit it as another patch after the initial stack lands?

SGTM as a TODO

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

clang/test/DebugInfo/KeyInstructions/switch.c

+// CHECK-CXX: sw.bb2:
+// CHECK-CXX: store i32 1, ptr %C{{.*}}, !dbg [[G4R1:!.*]]


Shouldn't this be testing for the switch getting a distinct group number too? For completeness if nothing else.

There's no switch emitted as it gets constant folded: https://godbolt.org/z/3vv4qxTMr

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

jmorse

LGTM

jmorse · 2025-05-23T16:06:07Z

clang/test/DebugInfo/KeyInstructions/switch.c

+void a(int A, int B) {
+// CHECK: entry:
+// The load gets associated with the branch rather than the store.
+// FIXME: Is that the best thing to do?


SGTM as a TODO

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

This was referenced Apr 7, 2025

[KeyInstr][Clang] Aggregate init + copy #134639

Merged

[KeyInstr][Clang] Catch variable init atom #134641

Open

OCHyams force-pushed the users/OCHyams/ki-clang-if branch from 98ea64f to 2fb58a6 Compare May 21, 2025 14:36

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

jmorse approved these changes May 22, 2025

View reviewed changes

Base automatically changed from users/OCHyams/ki-clang-if to users/OCHyams/ki-clang-catch-init May 23, 2025 09:15

OCHyams added 2 commits May 23, 2025 10:16

cc1

2e3a53b

OCHyams marked this pull request as draft May 23, 2025 09:17

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

OCHyams changed the base branch from users/OCHyams/ki-clang-catch-init to main May 23, 2025 09:17

OCHyams marked this pull request as ready for review May 23, 2025 09:18

jmorse approved these changes May 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[KeyInstr][Clang] Switch stmt atom #134643

[KeyInstr][Clang] Switch stmt atom #134643

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!

OCHyams May 23, 2025

Uh oh!

jmorse May 23, 2025

Uh oh!

jmorse May 22, 2025

Uh oh!

OCHyams May 23, 2025

Uh oh!

jmorse left a comment

Uh oh!

jmorse May 23, 2025

Uh oh!

Uh oh!

		// CHECK-CXX: sw.bb2:
		// CHECK-CXX: store i32 1, ptr %C{{.}}, !dbg [[G4R1:!.]]

[KeyInstr][Clang] Switch stmt atom #134643

Are you sure you want to change the base?

[KeyInstr][Clang] Switch stmt atom #134643

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!

OCHyams May 23, 2025

Choose a reason for hiding this comment

Uh oh!

jmorse May 23, 2025

Choose a reason for hiding this comment

Uh oh!

jmorse May 22, 2025

Choose a reason for hiding this comment

Uh oh!

OCHyams May 23, 2025

Choose a reason for hiding this comment

Uh oh!

jmorse left a comment

Choose a reason for hiding this comment

Uh oh!

jmorse May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

OCHyams commented Apr 7, 2025 •

edited

Loading