-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[KeyInstr][Clang] For stmt atom #134646
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.
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
base: main
Are you sure you want to change the base?
[KeyInstr][Clang] For stmt atom #134646
Conversation
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen 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/134646.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index d9fd406ad64ee..65b71c39d86c4 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -1324,6 +1324,7 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S,
Continue = getJumpDestInCurrentScope("for.inc");
BreakContinueStack.push_back(BreakContinue(LoopExit, Continue));
+ llvm::BasicBlock *ForBody = nullptr;
if (S.getCond()) {
// If the for statement has a condition scope, emit the local variable
// declaration.
@@ -1348,7 +1349,7 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S,
ExitBlock = createBasicBlock("for.cond.cleanup");
// As long as the condition is true, iterate the loop.
- llvm::BasicBlock *ForBody = createBasicBlock("for.body");
+ ForBody = createBasicBlock("for.body");
// C99 6.8.5p2/p4: The first substatement is executed if the expression
// compares unequal to 0. The condition must be a scalar type.
@@ -1362,7 +1363,14 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S,
BoolCondVal = emitCondLikelihoodViaExpectIntrinsic(
BoolCondVal, Stmt::getLikelihood(S.getBody()));
- Builder.CreateCondBr(BoolCondVal, ForBody, ExitBlock, Weights);
+ auto *I = Builder.CreateCondBr(BoolCondVal, ForBody, ExitBlock, Weights);
+ // Key Instructions: Emit the condition and branch as separate atoms to
+ // match existing loop stepping behaviour. FIXME: We could have the branch
+ // as the backup location for the condition, which would probably be a
+ // better experience (no jumping to the brace).
+ if (auto *I = dyn_cast<llvm::Instruction>(BoolCondVal))
+ addInstToNewSourceAtom(I, nullptr);
+ addInstToNewSourceAtom(I, nullptr);
if (ExitBlock != LoopExit.getBlock()) {
EmitBlock(ExitBlock);
@@ -1416,6 +1424,12 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S,
if (CGM.shouldEmitConvergenceTokens())
ConvergenceTokenStack.pop_back();
+
+ if (ForBody) {
+ // Key Instructions: We want the for closing brace to be step-able on to
+ // match existing behaviour.
+ addInstToNewSourceAtom(ForBody->getTerminator(), nullptr);
+ }
}
void
diff --git a/clang/test/KeyInstructions/for.c b/clang/test/KeyInstructions/for.c
new file mode 100644
index 0000000000000..3221ece69a717
--- /dev/null
+++ b/clang/test/KeyInstructions/for.c
@@ -0,0 +1,37 @@
+// RUN: %clang -gkey-instructions -x c++ %s -gmlt -S -emit-llvm -o - \
+// RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank
+
+// RUN: %clang -gkey-instructions -x c %s -gmlt -S -emit-llvm -o - \
+// RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank
+
+// Perennial quesiton: should the inc be its own source atom or not
+// (currently it is).
+
+// FIXME: See do.c and while.c regarding cmp and cond br groups.
+
+void a(int A) {
+// CHECK: entry:
+// CHECK: store i32 0, ptr %i{{.*}}, !dbg [[G1R1:!.*]]
+// CHECK: for.cond:
+// CHECK: %cmp = icmp slt i32 %0, %1, !dbg [[G2R1:!.*]]
+// CHECK: br i1 %cmp, label %for.body, label %for.end, !dbg [[G3R1:!.*]]
+
+// FIXME: Added uncond br group here which is useful for O0, which we're
+// no longer targeting. With optimisations loop rotate puts the condition
+// into for.inc and simplifycfg smooshes that and for.body together, so
+// it's not clear whether it adds any value.
+// CHECK: for.body:
+// CHECK: br label %for.inc, !dbg [[G5R1:!.*]]
+
+// CHECK: for.inc:
+// CHECK: %inc = add{{.*}}, !dbg [[G4R2:!.*]]
+// CHECK: store i32 %inc, ptr %i{{.*}}, !dbg [[G4R1:!.*]]
+ for (int i = 0; i < A; ++i) { }
+}
+
+// CHECK: [[G1R1]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1)
+// CHECK: [[G2R1]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 1)
+// CHECK: [[G3R1]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 1)
+// CHECK: [[G5R1]] = !DILocation({{.*}}, atomGroup: 5, atomRank: 1)
+// CHECK: [[G4R2]] = !DILocation({{.*}}, atomGroup: 4, atomRank: 2)
+// CHECK: [[G4R1]] = !DILocation({{.*}}, atomGroup: 4, atomRank: 1)
|
38f3eec
to
cdd6ff3
Compare
375ca9f
to
c4b2db1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question inline
clang/lib/CodeGen/CGStmt.cpp
Outdated
if (auto *I = dyn_cast<llvm::Instruction>(BoolCondVal)) | ||
addInstToNewSourceAtom(I, nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with prior patches, pls2rename I
to not shadow the outer I
clang/lib/CodeGen/CGStmt.cpp
Outdated
if (ForBody) { | ||
// Key Instructions: We want the for closing brace to be step-able on to | ||
// match existing behaviour. | ||
addInstToNewSourceAtom(ForBody->getTerminator(), nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that this works for the simple case, i.e. a for loop with a single block in it that is the body -- but what about the more complicated situation of more control flow in the for loop? Will the ForBody block have parts of that control flow in it?
If this is already handled (I imagine there's some abstraction in clang I'm unfamiliar with), testing for the correct behaviour would be good.
// FIXME: Added uncond br group here which is useful for O0, which we're | ||
// no longer targeting. With optimisations loop rotate puts the condition | ||
// into for.inc and simplifycfg smooshes that and for.body together, so | ||
// it's not clear whether it adds any value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this FIXME transcends my understanding
cdd6ff3
to
8b75d7f
Compare
2317236
to
74a6ebd
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
74a6ebd
to
b41dc1c
Compare
You were right about any ctrl-flow breaking the br-to-cond atom group. Fixed, improved testing, and follow up with additional work for continue/break in #141618 n.b. this is now based on top of #141607 so please ignore the first commit (sorry, the graphite stack has completely toppled and we're back into GitHub freeform stacking nastiness) |
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