https://github.com/OCHyams updated https://github.com/llvm/llvm-project/pull/134646
>From 0c7a83007e218dd7338b92f1d50319b0f35cf55f Mon Sep 17 00:00:00 2001 From: Orlando Cazalet-Hyams <orlando.hy...@sony.com> Date: Thu, 3 Apr 2025 19:12:47 +0100 Subject: [PATCH 1/7] [KeyInstr][Clang] For stmt atom 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: https://github.com/llvm/llvm-project/pull/130943 --- clang/lib/CodeGen/CGStmt.cpp | 18 +++++++++-- clang/test/DebugInfo/KeyInstructions/for.c | 37 ++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 clang/test/DebugInfo/KeyInstructions/for.c diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp index 31d64a5a788ee..0655e38cd4e4b 100644 --- a/clang/lib/CodeGen/CGStmt.cpp +++ b/clang/lib/CodeGen/CGStmt.cpp @@ -1326,6 +1326,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. @@ -1350,7 +1351,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. @@ -1364,7 +1365,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); @@ -1418,6 +1426,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/DebugInfo/KeyInstructions/for.c b/clang/test/DebugInfo/KeyInstructions/for.c new file mode 100644 index 0000000000000..3221ece69a717 --- /dev/null +++ b/clang/test/DebugInfo/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) >From 396ab05c30824352812599cd1ea9847387c7829c Mon Sep 17 00:00:00 2001 From: Orlando Cazalet-Hyams <orlando.hy...@sony.com> Date: Wed, 21 May 2025 15:50:28 +0100 Subject: [PATCH 2/7] cc1 --- clang/test/DebugInfo/KeyInstructions/for.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/test/DebugInfo/KeyInstructions/for.c b/clang/test/DebugInfo/KeyInstructions/for.c index 3221ece69a717..dbca87d73c293 100644 --- a/clang/test/DebugInfo/KeyInstructions/for.c +++ b/clang/test/DebugInfo/KeyInstructions/for.c @@ -1,7 +1,7 @@ -// RUN: %clang -gkey-instructions -x c++ %s -gmlt -S -emit-llvm -o - \ +// 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 -// RUN: %clang -gkey-instructions -x c %s -gmlt -S -emit-llvm -o - \ +// 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 // Perennial quesiton: should the inc be its own source atom or not >From 664478c9de6cd92741dd135b16cf76dacca79ed3 Mon Sep 17 00:00:00 2001 From: Orlando Cazalet-Hyams <orlando.hy...@sony.com> Date: Tue, 27 May 2025 11:30:33 +0100 Subject: [PATCH 3/7] nit: shadowed var, test triple --- clang/lib/CodeGen/CGStmt.cpp | 4 ++-- clang/test/DebugInfo/KeyInstructions/for.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp index 0655e38cd4e4b..bb58d69d41fc9 100644 --- a/clang/lib/CodeGen/CGStmt.cpp +++ b/clang/lib/CodeGen/CGStmt.cpp @@ -1370,8 +1370,8 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S, // 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); + if (auto *CondI = dyn_cast<llvm::Instruction>(BoolCondVal)) + addInstToNewSourceAtom(CondI, nullptr); addInstToNewSourceAtom(I, nullptr); if (ExitBlock != LoopExit.getBlock()) { diff --git a/clang/test/DebugInfo/KeyInstructions/for.c b/clang/test/DebugInfo/KeyInstructions/for.c index dbca87d73c293..c5f57f863f478 100644 --- a/clang/test/DebugInfo/KeyInstructions/for.c +++ b/clang/test/DebugInfo/KeyInstructions/for.c @@ -1,7 +1,7 @@ -// RUN: %clang_cc1 -gkey-instructions -x c++ %s -debug-info-kind=line-tables-only -emit-llvm -o - \ +// RUN: %clang_cc1 -triple x86_64-linux-gnu -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 -// RUN: %clang_cc1 -gkey-instructions -x c %s -debug-info-kind=line-tables-only -emit-llvm -o - \ +// RUN: %clang_cc1 -triple x86_64-linux-gnu -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 // Perennial quesiton: should the inc be its own source atom or not >From 446e622fef9850d15ed932f9c69081b1a1bc6f62 Mon Sep 17 00:00:00 2001 From: Orlando Cazalet-Hyams <orlando.hy...@sony.com> Date: Tue, 27 May 2025 11:33:53 +0100 Subject: [PATCH 4/7] improve test comments --- clang/test/DebugInfo/KeyInstructions/for.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/clang/test/DebugInfo/KeyInstructions/for.c b/clang/test/DebugInfo/KeyInstructions/for.c index c5f57f863f478..ac5411917d0c1 100644 --- a/clang/test/DebugInfo/KeyInstructions/for.c +++ b/clang/test/DebugInfo/KeyInstructions/for.c @@ -4,10 +4,10 @@ // RUN: %clang_cc1 -triple x86_64-linux-gnu -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 -// Perennial quesiton: should the inc be its own source atom or not +// Perennial question: 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. +// TODO: See do.c and while.c regarding cmp and cond br groups. void a(int A) { // CHECK: entry: @@ -16,10 +16,9 @@ void a(int A) { // 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. +// TODO: The unconditional br is given an atom group here which is useful for +// O0. Since we're no longer targeting O0 we should reevaluate whether this +// adds any value. // CHECK: for.body: // CHECK: br label %for.inc, !dbg [[G5R1:!.*]] >From 81005f97f119a753c20768474d7e3d4f8c60eb5c Mon Sep 17 00:00:00 2001 From: Orlando Cazalet-Hyams <orlando.hy...@sony.com> Date: Tue, 27 May 2025 14:34:48 +0100 Subject: [PATCH 5/7] improve test --- clang/test/DebugInfo/KeyInstructions/for.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/clang/test/DebugInfo/KeyInstructions/for.c b/clang/test/DebugInfo/KeyInstructions/for.c index ac5411917d0c1..bf0c7031d5071 100644 --- a/clang/test/DebugInfo/KeyInstructions/for.c +++ b/clang/test/DebugInfo/KeyInstructions/for.c @@ -20,17 +20,18 @@ void a(int A) { // O0. Since we're no longer targeting O0 we should reevaluate whether this // adds any value. // CHECK: for.body: -// CHECK: br label %for.inc, !dbg [[G5R1:!.*]] +// CHECK-NEXT: 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) { } + 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: [[G5R1]] = !DILocation(line: 29,{{.*}} atomGroup: 5, atomRank: 1) // CHECK: [[G4R2]] = !DILocation({{.*}}, atomGroup: 4, atomRank: 2) // CHECK: [[G4R1]] = !DILocation({{.*}}, atomGroup: 4, atomRank: 1) >From fe4a7d8a8cb80ac529a8e2847a488b1ccbc23b33 Mon Sep 17 00:00:00 2001 From: Orlando Cazalet-Hyams <orlando.hy...@sony.com> Date: Tue, 27 May 2025 15:22:11 +0100 Subject: [PATCH 6/7] fix for body with ctrl-flow --- clang/lib/CodeGen/CGStmt.cpp | 9 +-- clang/test/DebugInfo/KeyInstructions/for.c | 67 ++++++++++++++++++++++ 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp index bb58d69d41fc9..cd2f05d419216 100644 --- a/clang/lib/CodeGen/CGStmt.cpp +++ b/clang/lib/CodeGen/CGStmt.cpp @@ -1326,7 +1326,6 @@ 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. @@ -1351,7 +1350,7 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S, ExitBlock = createBasicBlock("for.cond.cleanup"); // As long as the condition is true, iterate the loop. - ForBody = createBasicBlock("for.body"); + llvm::BasicBlock *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. @@ -1397,6 +1396,8 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S, EmitStmt(S.getBody()); } + auto *FinalBodyBB = Builder.GetInsertBlock(); + // If there is an increment, emit it next. if (S.getInc()) { EmitBlock(Continue.getBlock()); @@ -1427,10 +1428,10 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S, if (CGM.shouldEmitConvergenceTokens()) ConvergenceTokenStack.pop_back(); - if (ForBody) { + if (FinalBodyBB) { // Key Instructions: We want the for closing brace to be step-able on to // match existing behaviour. - addInstToNewSourceAtom(ForBody->getTerminator(), nullptr); + addInstToNewSourceAtom(FinalBodyBB->getTerminator(), nullptr); } } diff --git a/clang/test/DebugInfo/KeyInstructions/for.c b/clang/test/DebugInfo/KeyInstructions/for.c index bf0c7031d5071..eafc08c655d57 100644 --- a/clang/test/DebugInfo/KeyInstructions/for.c +++ b/clang/test/DebugInfo/KeyInstructions/for.c @@ -29,9 +29,76 @@ void a(int A) { } } +void b(int A) { +// CHECK: entry: +// CHECK: store i32 0, ptr %i{{.*}}, !dbg [[bG1R1:!.*]] +// CHECK: for.cond: +// CHECK: %cmp = icmp slt i32 %0, %1, !dbg [[bG2R1:!.*]] +// CHECK: br i1 %cmp, label %for.body, label %for.end, !dbg [[bG3R1:!.*]] + +// CHECK: for.body: +// CHECK-NEXT: %2 = load i32, ptr %A.addr +// - If stmt atom: +// CHECK-NEXT: %cmp1 = icmp sgt i32 %2, 1, !dbg [[bG4R2:!.*]] +// CHECK-NEXT: br i1 %cmp1, label %if.then, label %if.end, !dbg [[bG4R1:!.*]] +// CHECK: if.then: +// CHECK-NEXT: br label %if.end + +// - For closing brace. +// CHECK: if.end: +// CHECK-NEXT: br label %for.inc, !dbg [[bG6R1:!.*]] + +// CHECK: for.inc: +// CHECK: %inc = add{{.*}}, !dbg [[bG5R2:!.*]] +// CHECK: store i32 %inc, ptr %i{{.*}}, !dbg [[bG5R1:!.*]] + for (int i = 0; i < A; ++i) { + if (A > 1) + ; + } +} + +void c(int A) { +// CHECK: entry: +// CHECK: for.cond: +// CHECK-NEXT: %0 = load i32, ptr %A.addr +// - If stmt atom: +// CHECK-NEXT: %cmp = icmp sgt i32 %0, 1, !dbg [[cG1R2:!.*]] +// CHECK-NEXT: br i1 %cmp, label %if.then, label %if.end, !dbg [[cG1R1:!.*]] +// CHECK: if.then: +// CHECK-NEXT: br label %if.end + +// - For closing brace. +// CHECK: if.end: +// CHECK-NEXT: br label %for.inc, !dbg [[cG3R1:!.*]] + +// CHECK: for.inc: +// CHECK-NEXT: %1 = load i32, ptr %A.addr +// CHECK-NEXT: %inc = add{{.*}}, !dbg [[cG2R2:!.*]] +// CHECK-NEXT: store i32 %inc, ptr %A.addr{{.*}}, !dbg [[cG2R1:!.*]] + for (; /*no cond*/ ; ++A) { + if (A > 1) + ; + } +} + // CHECK: [[G1R1]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1) // CHECK: [[G2R1]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 1) // CHECK: [[G3R1]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 1) // CHECK: [[G5R1]] = !DILocation(line: 29,{{.*}} atomGroup: 5, atomRank: 1) // CHECK: [[G4R2]] = !DILocation({{.*}}, atomGroup: 4, atomRank: 2) // CHECK: [[G4R1]] = !DILocation({{.*}}, atomGroup: 4, atomRank: 1) + +// CHECK: [[bG1R1]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1) +// CHECK: [[bG2R1]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 1) +// CHECK: [[bG3R1]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 1) +// CHECK: [[bG4R2]] = !DILocation({{.*}}, atomGroup: 4, atomRank: 2) +// CHECK: [[bG4R1]] = !DILocation({{.*}}, atomGroup: 4, atomRank: 1) +// CHECK: [[bG6R1]] = !DILocation(line: 57,{{.*}} atomGroup: 6, atomRank: 1) +// CHECK: [[bG5R2]] = !DILocation({{.*}}, atomGroup: 5, atomRank: 2) +// CHECK: [[bG5R1]] = !DILocation({{.*}}, atomGroup: 5, atomRank: 1) + +// CHECK: [[cG1R2]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 2) +// CHECK: [[cG1R1]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1) +// CHECK: [[cG3R1]] = !DILocation(line: 81,{{.*}} atomGroup: 3, atomRank: 1) +// CHECK: [[cG2R2]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 2) +// CHECK: [[cG2R1]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 1) \ No newline at end of file >From 0af9ceb763b8dd3d0d2d35f4e08948d4344654aa Mon Sep 17 00:00:00 2001 From: Orlando Cazalet-Hyams <orlando.hy...@sony.com> Date: Tue, 27 May 2025 15:44:25 +0100 Subject: [PATCH 7/7] improve test further: empty loop --- clang/test/DebugInfo/KeyInstructions/for.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/clang/test/DebugInfo/KeyInstructions/for.c b/clang/test/DebugInfo/KeyInstructions/for.c index eafc08c655d57..d1fc79292266b 100644 --- a/clang/test/DebugInfo/KeyInstructions/for.c +++ b/clang/test/DebugInfo/KeyInstructions/for.c @@ -81,6 +81,18 @@ void c(int A) { } } +void d() { +// - Check the `for` keyword gets an atom group. +// CHECK: entry: +// CHECK-NEXT: br label %for.cond + +// CHECK: for.cond: +// CHECK: br label %for.cond, !dbg [[dG1R1:!.*]], !llvm.loop + for ( ; ; ) + { + } +} + // CHECK: [[G1R1]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1) // CHECK: [[G2R1]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 1) // CHECK: [[G3R1]] = !DILocation({{.*}}, atomGroup: 3, atomRank: 1) @@ -101,4 +113,6 @@ void c(int A) { // CHECK: [[cG1R1]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1) // CHECK: [[cG3R1]] = !DILocation(line: 81,{{.*}} atomGroup: 3, atomRank: 1) // CHECK: [[cG2R2]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 2) -// CHECK: [[cG2R1]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 1) \ No newline at end of file +// CHECK: [[cG2R1]] = !DILocation({{.*}}, atomGroup: 2, atomRank: 1) + +// CHECK: [[dG1R1]] = !DILocation(line: 91, column: 3, scope: ![[#]], atomGroup: 1, atomRank: 1) _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
