Skip to content

[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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

OCHyams
Copy link
Contributor

@OCHyams OCHyams commented Apr 7, 2025

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 was referenced Apr 7, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 7, 2025

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

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


Full diff: https://github.com/llvm/llvm-project/pull/134646.diff

2 Files Affected:

  • (modified) clang/lib/CodeGen/CGStmt.cpp (+16-2)
  • (added) clang/test/KeyInstructions/for.c (+37)
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)

@OCHyams OCHyams force-pushed the users/OCHyams/ki-clang-while branch from 38f3eec to cdd6ff3 Compare May 21, 2025 14:47
@OCHyams OCHyams force-pushed the users/OCHyams/ki-clang-for branch from 375ca9f to c4b2db1 Compare May 21, 2025 14:50
Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

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

Question inline

Comment on lines 1371 to 1357
if (auto *I = dyn_cast<llvm::Instruction>(BoolCondVal))
addInstToNewSourceAtom(I, nullptr);
Copy link
Member

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

if (ForBody) {
// Key Instructions: We want the for closing brace to be step-able on to
// match existing behaviour.
addInstToNewSourceAtom(ForBody->getTerminator(), nullptr);
Copy link
Member

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.

Comment on lines 19 to 22
// 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.
Copy link
Member

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

@OCHyams OCHyams force-pushed the users/OCHyams/ki-clang-while branch from cdd6ff3 to 8b75d7f Compare May 23, 2025 13:32
Base automatically changed from users/OCHyams/ki-clang-while to main May 23, 2025 13:42
@OCHyams OCHyams marked this pull request as draft May 23, 2025 13:43
@OCHyams OCHyams force-pushed the users/OCHyams/ki-clang-for branch from 2317236 to 74a6ebd Compare May 23, 2025 13:43
@OCHyams OCHyams marked this pull request as ready for review May 23, 2025 13:43
OCHyams added 7 commits May 27, 2025 14:57
CGDebugInfo::completeFunction was added previously but mistakenly
not called (dropped through the cracks while putting together
the patch stack). Moved out of #134652 and #134654.
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 OCHyams force-pushed the users/OCHyams/ki-clang-for branch from 74a6ebd to b41dc1c Compare May 27, 2025 14:28
@OCHyams
Copy link
Contributor Author

OCHyams commented May 27, 2025

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants