Skip to content

[KeyInstr][Clang] Assignment atom group #134637

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
May 23, 2025
Merged

[KeyInstr][Clang] Assignment atom group #134637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5f5c88b
[KeyInstr][Clang] Assignment atom group
OCHyams Apr 2, 2025
a436d58
[KeyInstr][Clang] Multiple assignment (x = y = z)
OCHyams Apr 2, 2025
c7cf36a
Compound assign
OCHyams Apr 3, 2025
d19387f
Pre/Post Inc/Dec
OCHyams Apr 3, 2025
9c9e16b
Rename test & run C & C++
OCHyams Apr 3, 2025
9fe4199
%cc1 and rename a .cpp -> c test
OCHyams May 21, 2025
d2392bb
improve comment
OCHyams May 22, 2025
85b127c
test g += ++h;
OCHyams May 22, 2025
a96413e
rm accidental redundant test
OCHyams May 22, 2025
0f1b052
improve mem* checks in agg test
OCHyams May 22, 2025
c93251f
add comma operator coverage to test
OCHyams May 22, 2025
24fb5af
newline
OCHyams May 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions clang/lib/CodeGen/CGExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5981,6 +5981,15 @@ LValue CodeGenFunction::EmitBinaryOperatorLValue(const BinaryOperator *E) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assert(E->getOpcode() == BO_Assign && "unexpected binary l-value");

// Create a Key Instructions source location atom group that covers both
// LHS and RHS expressions. Nested RHS expressions may get subsequently
// separately grouped (1 below):
//
// 1. `a = b = c` -> Two atoms.
// 2. `x = new(1)` -> One atom (for both addr store and value store).
// 3. Complex and agg assignment -> One atom.
ApplyAtomGroup Grp(getDebugInfo());

// Note that in all of these cases, __block variables need the RHS
// evaluated first just in case the variable gets moved by the RHS.

Expand Down
4 changes: 4 additions & 0 deletions clang/lib/CodeGen/CGExprScalar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -898,6 +898,7 @@ class ScalarExprEmitter
return result; \
} \
Value *VisitBin##OP##Assign(const CompoundAssignOperator *E) { \
ApplyAtomGroup Grp(CGF.getDebugInfo()); \
return EmitCompoundAssign(E, &ScalarExprEmitter::Emit##OP); \
}
HANDLEBINOP(Mul)
Expand Down Expand Up @@ -3014,6 +3015,7 @@ class OMPLastprivateConditionalUpdateRAII {
llvm::Value *
ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV,
bool isInc, bool isPre) {
ApplyAtomGroup Grp(CGF.getDebugInfo());
OMPLastprivateConditionalUpdateRAII OMPRegion(CGF, E);
QualType type = E->getSubExpr()->getType();
llvm::PHINode *atomicPHI = nullptr;
Expand Down Expand Up @@ -5067,6 +5069,7 @@ llvm::Value *CodeGenFunction::EmitWithOriginalRHSBitfieldAssignment(
}

Value *ScalarExprEmitter::VisitBinAssign(const BinaryOperator *E) {
ApplyAtomGroup Grp(CGF.getDebugInfo());
bool Ignore = TestAndClearIgnoreResultAssign();

Value *RHS;
Expand Down Expand Up @@ -5849,6 +5852,7 @@ LValue CodeGenFunction::EmitObjCIsaExpr(const ObjCIsaExpr *E) {

LValue CodeGenFunction::EmitCompoundAssignmentLValue(
const CompoundAssignOperator *E) {
ApplyAtomGroup Grp(getDebugInfo());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

ScalarExprEmitter Scalar(*this);
Value *Result = nullptr;
switch (E->getOpcode()) {
Expand Down
85 changes: 85 additions & 0 deletions clang/test/DebugInfo/KeyInstructions/assign-scalar.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// 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
Comment on lines +1 to +2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


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

unsigned long long g, h, i;
void fun() {
// CHECK: store i64 0, ptr @g{{.*}}, !dbg [[G1R1:!.*]]
g = 0;

// Treat the two assignments as two atoms.
//
// FIXME: Because of the atomGroup implementation the load can only be
// associated with one of the two stores, despite being a good backup
// loction for both.
// CHECK-NEXT: %0 = load i64, ptr @g{{.*}}, !dbg [[G2R2:!.*]]
// CHECK-NEXT: store i64 %0, ptr @g{{.*}}, !dbg [[G3R1:!.*]]
// CHECK-NEXT: store i64 %0, ptr @g{{.*}}, !dbg [[G2R1:!.*]]
g = g = g;

// Compound assignment.
// CHECK: %1 = load i64, ptr @g
// CHECK: %add = add i64 %1, 50, !dbg [[G4R2:!.*]]
// CHECK: store i64 %add, ptr @g{{.*}}, !dbg [[G4R1:!.*]]
g += 50;

// Pre/Post Inc/Dec.
// CHECK: %2 = load i64, ptr @g
// CHECK: %inc = add i64 %2, 1, !dbg [[G5R2:!.*]]
// CHECK: store i64 %inc, ptr @g{{.*}}, !dbg [[G5R1:!.*]]
++g;
// CHECK: %3 = load i64, ptr @g
// CHECK: %dec = add i64 %3, -1, !dbg [[G6R2:!.*]]
// CHECK: store i64 %dec, ptr @g{{.*}}, !dbg [[G6R1:!.*]]
g--;

// Compound assignment with assignment on RHS, the assignments should have
// their own separate atom groups.
// CHECK-NEXT: %4 = load i64, ptr @h{{.*}}, !dbg [[load_h_loc:!.*]]
// CHECK-NEXT: %inc1 = add i64 %4, 1, !dbg [[G8R2:!.*]]
// CHECK-NEXT: store i64 %inc1, ptr @h{{.*}}, !dbg [[G8R1:!.*]]
// CHECK-NEXT: %5 = load i64, ptr @g{{.*}}, !dbg [[load_g_loc:!.*]]
// CHECK-NEXT: %add2 = add i64 %5, %inc1, !dbg [[G7R2:!.*]]
// CHECK-NEXT: store i64 %add2, ptr @g{{.*}}, !dbg [[G7R1:!.*]]
g += ++h;

// Double check the comma operator doesn't disturb atom groupings. There
// are three assignments, so we should get three groups.
// FIXME: Same situation as earlier in the test - because of the atomGroup
// implementation the load (from h) can only be associated with one of the two
// stores (to h and g) despite being a good backup location for both.
// CHECK-NEXT: %6 = load i64, ptr @h{{.*}}, !dbg [[load_h_loc2:!.*]]
// CHECK-NEXT: %inc3 = add i64 %6, 1, !dbg [[G9R2:!.*]]
// CHECK-NEXT: store i64 %inc3, ptr @h{{.*}}, !dbg [[G10R1:!.*]]
// CHECK-NEXT: store i64 %inc3, ptr @g{{.*}}, !dbg [[G9R1:!.*]]
// CHECK-NEXT: %7 = load i64, ptr @i{{.*}}, !dbg [[load_i_loc:!.*]]
// CHECK-NEXT: %inc4 = add i64 %7, 1, !dbg [[G11R2:!.*]]
// CHECK-NEXT: store i64 %inc4, ptr @i{{.*}}, !dbg [[G11R1:!.*]]
g = ++h, ++i;
}

// CHECK: [[G1R1]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1)
// CHECK: [[G2R2]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 2)
// CHECK: [[G3R1]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 1)
// CHECK: [[G2R1]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 1)
// CHECK: [[G4R2]] = !DILocation({{.*}}, atomGroup: 4, atomRank: 2)
// CHECK: [[G4R1]] = !DILocation({{.*}}, atomGroup: 4, atomRank: 1)
// CHECK: [[G5R2]] = !DILocation({{.*}}, atomGroup: 5, atomRank: 2)
// CHECK: [[G5R1]] = !DILocation({{.*}}, atomGroup: 5, atomRank: 1)
// CHECK: [[G6R2]] = !DILocation({{.*}}, atomGroup: 6, atomRank: 2)
// CHECK: [[G6R1]] = !DILocation({{.*}}, atomGroup: 6, atomRank: 1)
// CHECK: [[load_h_loc]] = !DILocation(line: [[#]], column: [[#]], scope: ![[#]])
// CHECK: [[G8R2]] = !DILocation({{.*}}, atomGroup: 8, atomRank: 2)
// CHECK: [[G8R1]] = !DILocation({{.*}}, atomGroup: 8, atomRank: 1)
// CHECK: [[load_g_loc]] = !DILocation(line: [[#]], column: [[#]], scope: ![[#]])
// CHECK: [[G7R2]] = !DILocation({{.*}}, atomGroup: 7, atomRank: 2)
// CHECK: [[G7R1]] = !DILocation({{.*}}, atomGroup: 7, atomRank: 1)
// CHECK: [[load_h_loc2]] = !DILocation(line: [[#]], column: [[#]], scope: ![[#]])
// CHECK: [[G9R2]] = !DILocation({{.*}}, atomGroup: 9, atomRank: 2)
// CHECK: [[G10R1]] = !DILocation({{.*}}, atomGroup: 10, atomRank: 1)
// CHECK: [[G9R1]] = !DILocation({{.*}}, atomGroup: 9, atomRank: 1)
// CHECK: [[load_i_loc]] = !DILocation(line: [[#]], column: [[#]], scope: ![[#]])
// CHECK: [[G11R2]] = !DILocation({{.*}}, atomGroup: 11, atomRank: 2)
// CHECK: [[G11R1]] = !DILocation({{.*}}, atomGroup: 11, atomRank: 1)
41 changes: 41 additions & 0 deletions clang/test/DebugInfo/KeyInstructions/init-agg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@

// RUN: %clang -gkey-instructions -x c++ %s -gmlt -gno-column-info -S -emit-llvm -o - -ftrivial-auto-var-init=pattern \
// RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank

// RUN: %clang -gkey-instructions -x c %s -gmlt -gno-column-info -S -emit-llvm -o - -ftrivial-auto-var-init=pattern \
// RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank

// The implicit-check-not is important; we don't want the GEPs created for the
// store locations to be included in the atom group.

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

// CHECK: store i32 1, ptr %{{.*}}, !dbg [[G2R1:!.*]]
// CHECK: store i32 2, ptr %{{.*}}, !dbg [[G2R1]]
// CHECK: %0 = load i32, ptr @g{{.*}}, !dbg [[G2R2:!.*]]
// CHECK: store i32 %0, ptr %{{.*}}, !dbg [[G2R1]]
int B[] = { 1, 2, g };

// CHECK: call void @llvm.memset{{.*}}%big{{.*}}, !dbg [[G3R1:!.*]]
// CHECK: store i8 97{{.*}}, !dbg [[G3R1]]
// CHECK: store i8 98{{.*}}, !dbg [[G3R1]]
// CHECK: store i8 99{{.*}}, !dbg [[G3R1]]
// CHECK: store i8 100{{.*}}, !dbg [[G3R1]]
char big[65536] = { 'a', 'b', 'c', 'd' };

// CHECK: call void @llvm.memset{{.*}}%arr{{.*}}, !dbg [[G4R1:!.*]]
char arr[] = { 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, };

// CHECK: store i8 -86, ptr %uninit{{.*}}, !dbg [[G5R1:!.*]], !annotation
char uninit; // -ftrivial-auto-var-init=pattern
}

// CHECK: [[G1R1]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1)
// CHECK: [[G2R1]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 1)
// CHECK: [[G2R2]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 2)
// CHECK: [[G3R1]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 1)
// CHECK: [[G4R1]] = !DILocation({{.*}}, atomGroup: 4, atomRank: 1)
// CHECK: [[G5R1]] = !DILocation({{.*}}, atomGroup: 5, atomRank: 1)