[KeyInstr][Clang] Assignment atom group#134637
Conversation
llvmbot
added
clang
OCHyams
force-pushed
the
users/OCHyams/ki-clang-init-static
branch
from
a92afbf to
76da803
Compare
OCHyams
force-pushed
the
users/OCHyams/ki-clang-assignment
branch
from
6788769 to
6f0135b
Compare
jmorse
left a comment
jmorse
left a comment
There was a problem hiding this comment.
Looks good, with a question inline and some test nits.
| @@ -5985,6 +5985,15 @@ LValue CodeGenFunction::EmitBinaryOperatorLValue(const BinaryOperator *E) { | |||
|
|
|||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeah that's right - added to the test for better coverage.
| // This covers both LHS and RHS expressions, though nested RHS | ||
| // expressions may get subsequently separately grouped. |
There was a problem hiding this comment.
Opening part of the comment should provide context to the reader that you're doing something debug-info / stepping related.
| // 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). |
There was a problem hiding this comment.
Is this TODO still relevant -- it's not clear what's actually to do.
There was a problem hiding this comment.
I was thinking out loud, I guess. Agree it doesn't really add anything (except confusion) so I've removed it.
|
|
||
| LValue CodeGenFunction::EmitCompoundAssignmentLValue( | ||
| const CompoundAssignOperator *E) { | ||
| ApplyAtomGroup Grp(getDebugInfo()); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
| // 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 |
There was a problem hiding this comment.
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.
| // CHECK: store i64 0, ptr @g{{.*}}, !dbg [[G1R1:!.*]] | ||
| g = 0; | ||
|
|
||
| // Treat the two assignments as two atoms. |
There was a problem hiding this comment.
Duplicate coverage with the g = g = g in the test file above?
There was a problem hiding this comment.
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).
| // CHECK: call void @llvm.memcpy{{.*}}, !dbg [[G1R1:!.*]] | ||
| int A[] = { 1, 2, 3 }; |
There was a problem hiding this comment.
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".
| // CHECK: store i32 %0, ptr %{{.*}}, !dbg [[G2R1]] | ||
| int B[] = { 1, 2, g }; | ||
|
|
||
| // CHECK: call void @llvm.memset{{.*}}, !dbg [[G3R1:!.*]] |
There was a problem hiding this comment.
Similarly, would be nice to have a more specific memset, and the one below.
|
Thanks @jmorse, that should be all nits/questions addressed now. |
OCHyams
force-pushed
the
users/OCHyams/ki-clang-init-static
branch
from
4e082dd to
ef9acc1
Compare
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
force-pushed
the
users/OCHyams/ki-clang-assignment
branch
from
d2d43d9 to
24fb5af
Compare
3 participants
[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