[KeyInstr][Clang] Scalar init atom #134633
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Orlando Cazalet-Hyams (OCHyams) ChangesThis patch is part of a stack that teaches Clang to generate Key Instructions The Key Instructions project is introduced, including a "quick summary" section The feature is only functional in LLVM if LLVM is built with CMake flag The Clang-side work is demoed here: Full diff: https://github.com/llvm/llvm-project/pull/134633.diff 3 Files Affected:
diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp
index eab1ebfb2369b..96d217b0a13cc 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -1921,6 +1921,7 @@ void CodeGenFunction::EmitAutoVarInit(const AutoVarEmission &emission) {
const VarDecl &D = *emission.Variable;
auto DL = ApplyDebugLocation::CreateDefaultArtificial(*this, D.getLocation());
+ ApplyAtomGroup Grp(getDebugInfo());
QualType type = D.getType();
// If this local has an initializer, emit it now.
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 5943ff9294e1a..a00a8ba2cd5cb 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -2180,6 +2180,8 @@ void CodeGenFunction::EmitStoreOfScalar(llvm::Value *Value, Address Addr,
}
llvm::StoreInst *Store = Builder.CreateStore(Value, Addr, Volatile);
+ addInstToCurrentSourceAtom(Store, Value);
+
if (isNontemporal) {
llvm::MDNode *Node =
llvm::MDNode::get(Store->getContext(),
diff --git a/clang/test/KeyInstructions/init-scalar.c b/clang/test/KeyInstructions/init-scalar.c
new file mode 100644
index 0000000000000..c212c2a4fd623
--- /dev/null
+++ b/clang/test/KeyInstructions/init-scalar.c
@@ -0,0 +1,19 @@
+// RUN: %clang -gkey-instructions -x c++ %s -gmlt -gcolumn-info -S -emit-llvm -o - \
+// RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank
+
+// RUN: %clang -gkey-instructions -x c %s -gmlt -gcolumn-info -S -emit-llvm -o - \
+// RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank
+
+void a() {
+// CHECK: store i32 0, ptr %A{{.*}}, !dbg [[G1R1:!.*]]
+ int A = 0;
+// CHECK: %add = add {{.*}}, !dbg [[G2R2:!.*]]
+// CHECK: store i32 %add, ptr %B, align 4, !dbg [[G2R1:!.*]]
+ int B = 2 * A + 1;
+// CHECK-TODO: ret{{.*}}, !dbg [[G3R1:!.*]]
+}
+
+// CHECK: [[G1R1]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1)
+// CHECK: [[G2R2]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 2)
+// CHECK: [[G2R1]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 1)
+// CHECK-TODO: [[G3R1]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 1)
|
There was a problem hiding this comment.
Looks good with a test tweak and question. My understanding of what's going on here is that:
- Assignment-expressions that store a scalar to memory are added to the "current" source atom,
- For source constructs that haven't been instrumented yet, that's potentially a no-op as the atom group will be zero,
- In this scenario of the initialization of an automatic variable, the group is set, and so the store gets the group number and rank.
I suppose this is a good way of decoupling the code that identifies the source construct that is "interesting" into the thing that calls ApplyAtomGroup, and the code that actually instruments instructions into the helper functions. That's got the upside that we don't have to think-about-and-manipulate the AST past an "interesting" construct; with the downside that things can leak out:
- "Uncovered" stores that aren't in an atom group, or that /would/ be in a "better" atom group but leak into an "outer" construct (as it were)
- Unexpectedly multiple stores entering an atom group?,
- Atom groups that unexpectedly don't cover anything.
...and thinking about all those different scenarios that can pop up, this feels like a good abstraction that avoids having to think about that. Maybe we don't cover every single construct / combination that's expressible, but this is all strictly an improvement in stepping anyway.
| // RUN: %clang -gkey-instructions -x c++ %s -gmlt -gcolumn-info -S -emit-llvm -o - \ | ||
| // RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank | ||
|
|
||
| // RUN: %clang -gkey-instructions -x c %s -gmlt -gcolumn-info -S -emit-llvm -o - \ | ||
| // RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank |
There was a problem hiding this comment.
I suspect we're supposed to use clang_cc1 here to avoid the driver interfering with core compiler Stuff (TM).
| // CHECK: %add = add {{.*}}, !dbg [[G2R2:!.*]] | ||
| // CHECK: store i32 %add, ptr %B, align 4, !dbg [[G2R1:!.*]] | ||
| int B = 2 * A + 1; | ||
| // CHECK-TODO: ret{{.*}}, !dbg [[G3R1:!.*]] |
There was a problem hiding this comment.
Ah, that's further up in the patch stack when rets get atom info.
There was a problem hiding this comment.
Removed the CHECK-TODOs, to avoid confusion. The checks get added later on and aren't needed for this patch.
OCHyams
force-pushed
the
users/OCHyams/ki-clang-scoped-setter
branch
2 times, most recently
from
0544aed to
8e7f643
Compare
OCHyams
force-pushed
the
users/OCHyams/ki-clang-init
branch
from
29d5428 to
2935186
Compare
That's right.
It's possible yep. OTOH, there are times we want to do that on purposes (aggregate init), so it's an important use case.
This is indeed a fear, as there's many ways to create a store/store-like-intrinsic that don't go through the instrumented functions. This current stack takes the approach of trying to instrument every one of those sites. Another approach is to do it inside the builder class so that all call sites are automatically covered. Then instead of "opt in" we'd need a mechanism for some call sites to "opt out" which feels overall probably safer. I was going to play around with the idea once all these patches land, as we'll have good test coverage by then.
I've addressed the inline comments (which have been eaten by github) - how does this look now? |
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-init
branch
from
2935186 to
024c670
Compare
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.
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.
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