[KeyInstr][Clang] For range stmt atoms

[OCHyams]

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]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• [NFC][KeyInstr][Clang] Add some additional tests #134654
• [KeyInstr][Clang] Coerced store atoms #134653
• [KeyInstr][Clang] Ret atom #134652
• [KeyIntsr][Clang] Builtins alloca auto-init atom #134651
• [KeyInstr][Clang] Assign matrix element atom #134650
• [KeyInstr][Clang] Assign vector element atom #134649
• [KeyInstr][Clang] Bitfield atom #134648
• [KeyInstr][Clang] For range stmt atoms #134647 👈 (View in Graphite)
• [KeyInstr][Clang] For stmt atom #134646
• [KeyInstr][Clang] While stmt atom #134645
• [KeyInstr][Clang] Do stmt atom #134644
• [KeyInstr][Clang] Switch stmt atom #134643
• [KeyInstr][Clang] If stmt atom #134642
• [KeyInstr][Clang] Catch variable init atom #134641
• [KeyInstr][Clang] Member initalization atom #134640
• [KeyInstr][Clang] Aggregate init + copy #134639
• [KeyInstr] Complex assignment atoms #134638
• [KeyInstr][Clang] Assignment atom group #134637
• [KeyInstr][Clang] Static variable init atom #134636
• [KeyInstr][Clang] Agg init atom #134635
• [Clang][NFC] Move some static functions into CodeGenFunction #134634
• [KeyInstr][Clang] Scalar init atom #134633
• [KeyInstr][Clang] Add ApplyAtomGroup #134632
• [KeyInstr][Clang] Add Clang option -g[no-]key-instructions #134627
• [KeyInstr][DwarfDebug] Add is_stmt emission support #133495
• [KeyInstr] Add MIR parser support #133494
• [KeyInstr] Remap cloned PHIs in BreakCriticalEdges #133493
• Propagate DebugLocs on phis in BreakCriticalEdges #133492
• [KeyInstr][LoopUnswitch] Remap cloned instructions' atoms #133491
• [KeyInstr][LoopRotate] Remap atoms of duplicated instructions #133490
• [KeyInstr][LoopUnroll] Remap atoms while unrolling #133489
• [KeyInstr][JumpThreading] Remap atoms duping bb with cond br on phi into pred #133488
• [KeyInstr][JumpThreading] Remap atoms after threading #133487
• [KeyInstr][JumpThreading] Remap atoms in blocks duplicated for threading #133486
• [KeyInstr][Inline] Don't propagate atoms to inlined nodebug instructions #133485
• [KeyInstr][SimplifyCFG] Remap atoms after duplication for threading #133484
• [KeyInstr][debugify] Add --debugify-atoms to add key instructions metadata #133483
• [KeyInstr][SimplifyCFG] Remap atoms when folding br to common succ into pred #133482
• [KeyInstr] Inline atom info #133481
• [KeyInstr] Merge atoms in DILocation::getMergedLocation #133480
• [NFC][KeyInstr] Add Atom Group (re)mapping #133479
• [KeyInstr] Add Atom Group waterline to LLVMContext #133478
• [KeyInstr] Add fields to DILocation behind compile time flag #133477
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 375ca9f7b756d01da82772f9316ab20b56214d91 2484a49c8f259891882a18636b769bf4c06cb8ce --extensions cpp -- clang/test/KeyInstructions/for-range.cpp clang/lib/CodeGen/CGStmt.cpp

View the diff from clang-format here.

diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index 9292be24fc..2696bc9cc3 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -1535,7 +1535,7 @@ CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
     ConvergenceTokenStack.pop_back();
 
   // We want the for closing brace to be step-able on to match existing
-  // behaviour. 
+  // behaviour.
   addInstToNewSourceAtom(ForBody->getTerminator(), nullptr);
 }
 

[llvmbot]

@llvm/pr-subscribers-clang

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/134647.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CGStmt.cpp (+12-1)
• (added) clang/test/KeyInstructions/for-range.cpp (+72)

diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index 65b71c39d86c4..9292be24fc12e 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -1477,7 +1477,14 @@ CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
   if (!Weights && CGM.getCodeGenOpts().OptimizationLevel)
     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.
+  if (auto *I = dyn_cast<llvm::Instruction>(BoolCondVal))
+    addInstToNewSourceAtom(I, nullptr);
+  addInstToNewSourceAtom(I, nullptr);
 
   if (ExitBlock != LoopExit.getBlock()) {
     EmitBlock(ExitBlock);
@@ -1526,6 +1533,10 @@ CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
 
   if (CGM.shouldEmitConvergenceTokens())
     ConvergenceTokenStack.pop_back();
+
+  // We want the for closing brace to be step-able on to match existing
+  // behaviour. 
+  addInstToNewSourceAtom(ForBody->getTerminator(), nullptr);
 }
 
 void CodeGenFunction::EmitReturnOfRValue(RValue RV, QualType Ty) {
diff --git a/clang/test/KeyInstructions/for-range.cpp b/clang/test/KeyInstructions/for-range.cpp
new file mode 100644
index 0000000000000..be71d9fbfb581
--- /dev/null
+++ b/clang/test/KeyInstructions/for-range.cpp
@@ -0,0 +1,72 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 5
+// RUN: %clang -gkey-instructions %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.
+
+// The stores in the setup (stores to __RANGE1, __BEGIN1, __END1) are all
+// marked as Key. Unclear whether that's desirable. Keep for now as that's
+// least risky.
+
+// Check the conditional branch (and the condition) in FOR_COND and
+// unconditional branch in FOR_BODY are Key Instructions.
+
+struct Range {
+    int *begin();
+    int *end();
+} r;
+
+// CHECK-LABEL: define dso_local void @_Z1av(
+// CHECK-SAME: ) #[[ATTR0:[0-9]+]] !dbg [[DBG10:![0-9]+]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[__RANGE1:%.*]] = alloca ptr, align 8
+// CHECK-NEXT:    [[__BEGIN1:%.*]] = alloca ptr, align 8
+// CHECK-NEXT:    [[__END1:%.*]] = alloca ptr, align 8
+// CHECK-NEXT:    [[I:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    store ptr @r, ptr [[__RANGE1]], align 8, !dbg [[DBG14:![0-9]+]]
+// CHECK-NEXT:    [[CALL:%.*]] = call noundef ptr @_ZN5Range5beginEv(ptr noundef nonnull align 1 dereferenceable(1) @r), !dbg [[DBG15:![0-9]+]]
+// CHECK-NEXT:    store ptr [[CALL]], ptr [[__BEGIN1]], align 8, !dbg [[DBG16:![0-9]+]]
+// CHECK-NEXT:    [[CALL1:%.*]] = call noundef ptr @_ZN5Range3endEv(ptr noundef nonnull align 1 dereferenceable(1) @r), !dbg [[DBG17:![0-9]+]]
+// CHECK-NEXT:    store ptr [[CALL1]], ptr [[__END1]], align 8, !dbg [[DBG18:![0-9]+]]
+// CHECK-NEXT:    br label %[[FOR_COND:.*]], !dbg [[DBG19:![0-9]+]]
+// CHECK:       [[FOR_COND]]:
+// CHECK-NEXT:    [[TMP0:%.*]] = load ptr, ptr [[__BEGIN1]], align 8, !dbg [[DBG19]]
+// CHECK-NEXT:    [[TMP1:%.*]] = load ptr, ptr [[__END1]], align 8, !dbg [[DBG19]]
+// CHECK-NEXT:    [[CMP:%.*]] = icmp ne ptr [[TMP0]], [[TMP1]], !dbg [[DBG20:![0-9]+]]
+// CHECK-NEXT:    br i1 [[CMP]], label %[[FOR_BODY:.*]], label %[[FOR_END:.*]], !dbg [[DBG21:![0-9]+]]
+// CHECK:       [[FOR_BODY]]:
+// CHECK-NEXT:    [[TMP2:%.*]] = load ptr, ptr [[__BEGIN1]], align 8, !dbg [[DBG19]]
+// CHECK-NEXT:    [[TMP3:%.*]] = load i32, ptr [[TMP2]], align 4, !dbg [[DBG22:![0-9]+]]
+// CHECK-NEXT:    store i32 [[TMP3]], ptr [[I]], align 4, !dbg [[DBG23:![0-9]+]]
+// CHECK-NEXT:    br label %[[FOR_INC:.*]], !dbg [[DBG24:![0-9]+]]
+// CHECK:       [[FOR_INC]]:
+// CHECK-NEXT:    [[TMP4:%.*]] = load ptr, ptr [[__BEGIN1]], align 8, !dbg [[DBG19]]
+// CHECK-NEXT:    [[INCDEC_PTR:%.*]] = getelementptr inbounds nuw i32, ptr [[TMP4]], i32 1, !dbg [[DBG25:![0-9]+]]
+// CHECK-NEXT:    store ptr [[INCDEC_PTR]], ptr [[__BEGIN1]], align 8, !dbg [[DBG26:![0-9]+]]
+// CHECK-NEXT:    br label %[[FOR_COND]], !dbg [[DBG19]], !llvm.loop
+// CHECK:       [[FOR_END]]:
+// CHECK-NEXT:    ret void, !dbg [[DBG30:![0-9]+]]
+//
+void a() {
+    for (int i: r)
+        ;
+}
+//.
+// CHECK: [[DBG14]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1)
+// CHECK: [[DBG15]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 2)
+// CHECK: [[DBG16]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 1)
+// CHECK: [[DBG17]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 2)
+// CHECK: [[DBG18]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 1)
+// CHECK: [[DBG19]] = !DILocation({{.*}})
+// CHECK: [[DBG20]] = !DILocation({{.*}}, atomGroup: 4, atomRank: 1)
+// CHECK: [[DBG21]] = !DILocation({{.*}}, atomGroup: 5, atomRank: 1)
+// CHECK: [[DBG22]] = !DILocation({{.*}}, atomGroup: 6, atomRank: 2)
+// CHECK: [[DBG23]] = !DILocation({{.*}}, atomGroup: 6, atomRank: 1)
+// CHECK: [[DBG24]] = !DILocation({{.*}} atomGroup: 8, atomRank: 1)
+// CHECK: [[DBG25]] = !DILocation({{.*}}, atomGroup: 7, atomRank: 2)
+// CHECK: [[DBG26]] = !DILocation({{.*}}, atomGroup: 7, atomRank: 1)
+// CHECK: [[DBG30]] = !DILocation({{.*}})
+//.

[llvmbot]

@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/134647.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CGStmt.cpp (+12-1)
• (added) clang/test/KeyInstructions/for-range.cpp (+72)

diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index 65b71c39d86c4..9292be24fc12e 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -1477,7 +1477,14 @@ CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
   if (!Weights && CGM.getCodeGenOpts().OptimizationLevel)
     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.
+  if (auto *I = dyn_cast<llvm::Instruction>(BoolCondVal))
+    addInstToNewSourceAtom(I, nullptr);
+  addInstToNewSourceAtom(I, nullptr);
 
   if (ExitBlock != LoopExit.getBlock()) {
     EmitBlock(ExitBlock);
@@ -1526,6 +1533,10 @@ CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
 
   if (CGM.shouldEmitConvergenceTokens())
     ConvergenceTokenStack.pop_back();
+
+  // We want the for closing brace to be step-able on to match existing
+  // behaviour. 
+  addInstToNewSourceAtom(ForBody->getTerminator(), nullptr);
 }
 
 void CodeGenFunction::EmitReturnOfRValue(RValue RV, QualType Ty) {
diff --git a/clang/test/KeyInstructions/for-range.cpp b/clang/test/KeyInstructions/for-range.cpp
new file mode 100644
index 0000000000000..be71d9fbfb581
--- /dev/null
+++ b/clang/test/KeyInstructions/for-range.cpp
@@ -0,0 +1,72 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 5
+// RUN: %clang -gkey-instructions %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.
+
+// The stores in the setup (stores to __RANGE1, __BEGIN1, __END1) are all
+// marked as Key. Unclear whether that's desirable. Keep for now as that's
+// least risky.
+
+// Check the conditional branch (and the condition) in FOR_COND and
+// unconditional branch in FOR_BODY are Key Instructions.
+
+struct Range {
+    int *begin();
+    int *end();
+} r;
+
+// CHECK-LABEL: define dso_local void @_Z1av(
+// CHECK-SAME: ) #[[ATTR0:[0-9]+]] !dbg [[DBG10:![0-9]+]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[__RANGE1:%.*]] = alloca ptr, align 8
+// CHECK-NEXT:    [[__BEGIN1:%.*]] = alloca ptr, align 8
+// CHECK-NEXT:    [[__END1:%.*]] = alloca ptr, align 8
+// CHECK-NEXT:    [[I:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    store ptr @r, ptr [[__RANGE1]], align 8, !dbg [[DBG14:![0-9]+]]
+// CHECK-NEXT:    [[CALL:%.*]] = call noundef ptr @_ZN5Range5beginEv(ptr noundef nonnull align 1 dereferenceable(1) @r), !dbg [[DBG15:![0-9]+]]
+// CHECK-NEXT:    store ptr [[CALL]], ptr [[__BEGIN1]], align 8, !dbg [[DBG16:![0-9]+]]
+// CHECK-NEXT:    [[CALL1:%.*]] = call noundef ptr @_ZN5Range3endEv(ptr noundef nonnull align 1 dereferenceable(1) @r), !dbg [[DBG17:![0-9]+]]
+// CHECK-NEXT:    store ptr [[CALL1]], ptr [[__END1]], align 8, !dbg [[DBG18:![0-9]+]]
+// CHECK-NEXT:    br label %[[FOR_COND:.*]], !dbg [[DBG19:![0-9]+]]
+// CHECK:       [[FOR_COND]]:
+// CHECK-NEXT:    [[TMP0:%.*]] = load ptr, ptr [[__BEGIN1]], align 8, !dbg [[DBG19]]
+// CHECK-NEXT:    [[TMP1:%.*]] = load ptr, ptr [[__END1]], align 8, !dbg [[DBG19]]
+// CHECK-NEXT:    [[CMP:%.*]] = icmp ne ptr [[TMP0]], [[TMP1]], !dbg [[DBG20:![0-9]+]]
+// CHECK-NEXT:    br i1 [[CMP]], label %[[FOR_BODY:.*]], label %[[FOR_END:.*]], !dbg [[DBG21:![0-9]+]]
+// CHECK:       [[FOR_BODY]]:
+// CHECK-NEXT:    [[TMP2:%.*]] = load ptr, ptr [[__BEGIN1]], align 8, !dbg [[DBG19]]
+// CHECK-NEXT:    [[TMP3:%.*]] = load i32, ptr [[TMP2]], align 4, !dbg [[DBG22:![0-9]+]]
+// CHECK-NEXT:    store i32 [[TMP3]], ptr [[I]], align 4, !dbg [[DBG23:![0-9]+]]
+// CHECK-NEXT:    br label %[[FOR_INC:.*]], !dbg [[DBG24:![0-9]+]]
+// CHECK:       [[FOR_INC]]:
+// CHECK-NEXT:    [[TMP4:%.*]] = load ptr, ptr [[__BEGIN1]], align 8, !dbg [[DBG19]]
+// CHECK-NEXT:    [[INCDEC_PTR:%.*]] = getelementptr inbounds nuw i32, ptr [[TMP4]], i32 1, !dbg [[DBG25:![0-9]+]]
+// CHECK-NEXT:    store ptr [[INCDEC_PTR]], ptr [[__BEGIN1]], align 8, !dbg [[DBG26:![0-9]+]]
+// CHECK-NEXT:    br label %[[FOR_COND]], !dbg [[DBG19]], !llvm.loop
+// CHECK:       [[FOR_END]]:
+// CHECK-NEXT:    ret void, !dbg [[DBG30:![0-9]+]]
+//
+void a() {
+    for (int i: r)
+        ;
+}
+//.
+// CHECK: [[DBG14]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1)
+// CHECK: [[DBG15]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 2)
+// CHECK: [[DBG16]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 1)
+// CHECK: [[DBG17]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 2)
+// CHECK: [[DBG18]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 1)
+// CHECK: [[DBG19]] = !DILocation({{.*}})
+// CHECK: [[DBG20]] = !DILocation({{.*}}, atomGroup: 4, atomRank: 1)
+// CHECK: [[DBG21]] = !DILocation({{.*}}, atomGroup: 5, atomRank: 1)
+// CHECK: [[DBG22]] = !DILocation({{.*}}, atomGroup: 6, atomRank: 2)
+// CHECK: [[DBG23]] = !DILocation({{.*}}, atomGroup: 6, atomRank: 1)
+// CHECK: [[DBG24]] = !DILocation({{.*}} atomGroup: 8, atomRank: 1)
+// CHECK: [[DBG25]] = !DILocation({{.*}}, atomGroup: 7, atomRank: 2)
+// CHECK: [[DBG26]] = !DILocation({{.*}}, atomGroup: 7, atomRank: 1)
+// CHECK: [[DBG30]] = !DILocation({{.*}})
+//.