[KeyInstr][Clang] Assignment atom group #134637

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

[KeyInstr][Clang] Assignment atom group

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

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

jmorse

Looks good, with a question inline and some test nits.

jmorse · 2025-05-21T16:44:30Z

clang/lib/CodeGen/CGExpr.cpp

@@ -5985,6 +5985,15 @@ LValue CodeGenFunction::EmitBinaryOperatorLValue(const BinaryOperator *E) {



I wonder whether the comma operator (six or seven lines above here) needs instrumenting -- technically if either lhs/rhs of the comma is an assignment, I guess it'll come back through this function and be given a separate group?

Yeah that's right - added to the test for better coverage.

jmorse · 2025-05-21T16:45:09Z

clang/lib/CodeGen/CGExpr.cpp

+  // This covers both LHS and RHS expressions, though nested RHS
+  // expressions may get subsequently separately grouped.


Opening part of the comment should provide context to the reader that you're doing something debug-info / stepping related.

jmorse · 2025-05-21T16:45:46Z

clang/lib/CodeGen/CGExpr.cpp

+  // FIXME(OCH): Not clear yet if we've got fine enough control
+  // to pick and choose when we need to. Currently looks ok:
+  // a = b = c  -> Two atoms.
+  // x = new(1) -> One atom (for both addr store and value store).


Is this TODO still relevant -- it's not clear what's actually to do.

I was thinking out loud, I guess. Agree it doesn't really add anything (except confusion) so I've removed it.

jmorse · 2025-05-21T16:51:43Z

clang/lib/CodeGen/CGExprScalar.cpp

@@ -5849,6 +5852,7 @@ LValue CodeGenFunction::EmitObjCIsaExpr(const ObjCIsaExpr *E) {

 LValue CodeGenFunction::EmitCompoundAssignmentLValue(
                                            const CompoundAssignOperator *E) {
+  ApplyAtomGroup Grp(getDebugInfo());


If I enter via a VisitBin##OP##Assign function from above I'll get an atom group before calling EmitCompoundAssign, then get a second atom group here in EmitcompoundAssignmentLValue (which is called by EmitCompoundAssign). I would have expected only one atom group to be generated in that, is that wrong?

If I enter via a VisitBin##OP##Assign function from above I'll get an atom group before calling EmitCompoundAssign,

Looks correct

then get a second atom group here in EmitcompoundAssignmentLValue (which is called by EmitCompoundAssign). I would have expected only one atom group to be generated in that, is that wrong?

Looks wrong - EmitCompoundAssign calls EmitCompoundAssignLValue not EmitcompoundAssignmentLValue. The function names are annoyingly similar, this took a bit of head-scratching to re-figure out!

jmorse · 2025-05-21T17:01:00Z

clang/test/DebugInfo/KeyInstructions/assign-scalar.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


This test is great; I feel we can make it even greater by testing:

g += ++h;

Or something like that, where there are stores on both sides of a compound assignment expression.

jmorse · 2025-05-21T17:01:53Z

clang/test/DebugInfo/KeyInstructions/assign.c

+// CHECK: store i64 0, ptr @g{{.*}}, !dbg [[G1R1:!.*]]
+    g = 0;
+
+// Treat the two assignments as two atoms.


Duplicate coverage with the g = g = g in the test file above?

Oops, this file magically appeared as a result of a wobbly rebase after moving the tests to the new dir. Deleted it, it is all completely redundant (clang/test/DebugInfo/KeyInstructions/assign-scalar.c covers exactly this and more).

jmorse · 2025-05-21T17:04:50Z

clang/test/DebugInfo/KeyInstructions/init-agg.c

+// CHECK: call void @llvm.memcpy{{.*}}, !dbg [[G1R1:!.*]]
+    int A[] = { 1, 2, 3 };


As with a prior review, tying the memcpy down to its destination SSA name would be nice, as it'd be more specific than "there is a memcpy with group 1 rank 1, anywhere".

Agree, done.

jmorse · 2025-05-21T17:06:13Z

clang/test/DebugInfo/KeyInstructions/init-agg.c

+// CHECK: store i32 %0, ptr %{{.*}}, !dbg [[G2R1]]
+    int B[] = { 1, 2, g };
+
+// CHECK: call void @llvm.memset{{.*}}, !dbg [[G3R1:!.*]]


Similarly, would be nice to have a more specific memset, and the one below.

OCHyams · 2025-05-22T14:55:07Z

Thanks @jmorse, that should be all nits/questions addressed now.

jmorse

LGTM

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 patch is part of a stack that teaches Clang to generate Key Instructions metadata for C and C++. RFC: 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.

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] Complex assignment atoms #134638

Open

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

Merged

OCHyams force-pushed the users/OCHyams/ki-clang-init-static branch from a92afbf to 76da803 Compare May 21, 2025 13:41

OCHyams force-pushed the users/OCHyams/ki-clang-assignment branch from 6788769 to 6f0135b Compare May 21, 2025 13:55

jmorse reviewed May 21, 2025

View reviewed changes

jmorse approved these changes May 22, 2025

View reviewed changes

OCHyams force-pushed the users/OCHyams/ki-clang-init-static branch from 4e082dd to ef9acc1 Compare May 22, 2025 16:07

Base automatically changed from users/OCHyams/ki-clang-init-static to main May 22, 2025 16:07

OCHyams added 12 commits May 22, 2025 17:18

[KeyInstr][Clang] Multiple assignment (x = y = z)

a436d58

Compound assign

c7cf36a

Pre/Post Inc/Dec

d19387f

Rename test & run C & C++

9c9e16b

%cc1 and rename a .cpp -> c test

9fe4199

improve comment

d2392bb

test g += ++h;

85b127c

rm accidental redundant test

a96413e

improve mem* checks in agg test

0f1b052

add comma operator coverage to test

c93251f

newline

24fb5af

OCHyams force-pushed the users/OCHyams/ki-clang-assignment branch from d2d43d9 to 24fb5af Compare May 22, 2025 16:18

OCHyams merged commit 8f1d1dd into main May 23, 2025
11 checks passed

OCHyams deleted the users/OCHyams/ki-clang-assignment branch May 23, 2025 08:40

		@@ -5985,6 +5985,15 @@ LValue CodeGenFunction::EmitBinaryOperatorLValue(const BinaryOperator *E) {

		// This covers both LHS and RHS expressions, though nested RHS
		// expressions may get subsequently separately grouped.

		// 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

		// CHECK: call void @llvm.memcpy{{.}}, !dbg [[G1R1:!.]]
		int A[] = { 1, 2, 3 };

[KeyInstr][Clang] Assignment atom group #134637

[KeyInstr][Clang] Assignment atom group #134637

Uh oh!

Conversation

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

Uh oh!

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!

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!

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!

OCHyams commented May 22, 2025

Uh oh!

jmorse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

OCHyams commented Apr 7, 2025 •

edited

Loading

OCHyams commented Apr 7, 2025 •

edited

Loading