[KeyInstr] Enable -gkey-instructions by default if optimisations are enabled #149509
Conversation
…enabled Key Instructions improves the optimized-code debug-stepping experience in debuggers that use DWARF's `is_stmt` line table register to determine stepping behaviour. The feature can be disabled with `-gno-key-instructions`. RFC: https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Orlando Cazalet-Hyams (OCHyams) ChangesThat's enabling Clang's -gkey-instructions, cc1's -gkey-instructions remains off by default. Key Instructions improves the optimized-code debug-stepping experience in debuggers that use DWARF's The feature can be disabled with RFC: https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668 Questions: LLDB - I believe LLDB ignores Clang's Clang languages/extensions - I've implemented support for C/C++, but I've not tested or specifically implemented anything for other languages (?) or extensions (?) like CUDA. Is there a good way to filter for this in the driver code in this patch? Full diff: https://github.com/llvm/llvm-project/pull/149509.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 8880c9375143f..38eebbd6f9b5f 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -4611,8 +4611,14 @@ renderDebugOptions(const ToolChain &TC, const Driver &D, const llvm::Triple &T,
CmdArgs.push_back("-gembed-source");
}
+ // Enable Key Instructions by default if optimisations are enabled and
+ // we're emitting DWARF.
+ Arg *OptLevel = Args.getLastArg(options::OPT_O_Group);
+ bool KeyInstructionsOnByDefault =
+ EmitDwarf && OptLevel && !OptLevel->getOption().matches(options::OPT_O0);
if (Args.hasFlag(options::OPT_gkey_instructions,
- options::OPT_gno_key_instructions, false))
+ options::OPT_gno_key_instructions,
+ KeyInstructionsOnByDefault))
CmdArgs.push_back("-gkey-instructions");
if (EmitCodeView) {
diff --git a/clang/test/DebugInfo/KeyInstructions/flag.cpp b/clang/test/DebugInfo/KeyInstructions/flag.cpp
index e34faa6cbb347..05a34ac670feb 100644
--- a/clang/test/DebugInfo/KeyInstructions/flag.cpp
+++ b/clang/test/DebugInfo/KeyInstructions/flag.cpp
@@ -1,7 +1,5 @@
// RUN: %clang -### -target x86_64 -c -gdwarf -gkey-instructions %s 2>&1 | FileCheck %s --check-prefixes=KEY-INSTRUCTIONS
// RUN: %clang -### -target x86_64 -c -gdwarf -gno-key-instructions %s 2>&1 | FileCheck %s --check-prefixes=NO-KEY-INSTRUCTIONS
-//// Default: Off.
-// RUN: %clang -### -target x86_64 -c -gdwarf %s 2>&1 | FileCheck %s --check-prefixes=NO-KEY-INSTRUCTIONS
//// Help.
// RUN %clang --help | FileCheck %s --check-prefix=HELP
@@ -23,3 +21,34 @@ void f() {}
// RUN: %clang_cc1 %s -triple x86_64-linux-gnu -gkey-instructions -debug-info-kind=line-tables-only -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-ON
// SMOKETEST-ON: keyInstructions: true
// SMOKETEST-ON: atomGroup: 1
+
+//// Enable Key Instructions by default if optimisations are enabled and we're
+//// emitting DWARF.
+////
+//// | opt level | -gkey-instructions | feature |
+//// | 0 | no | off |
+//// | 0 | yes | on |
+//// | >=1 | no | on |
+//// | >=1 | yes | on |
+//// | >=1 | no & no -g flags | off |
+//// | >=1 | no & emit codeview | off |
+//
+// RUN: %clang %s -target x86_64 -gdwarf -gmlt -### 2>&1 | FileCheck %s --check-prefix=NO-KEY-INSTRUCTIONS
+// RUN: %clang %s -target x86_64 -gdwarf -gmlt -gkey-instructions -### 2>&1 | FileCheck %s --check-prefix=KEY-INSTRUCTIONS
+// RUN: %clang %s -O0 -target x86_64 -gdwarf -gmlt -### 2>&1 | FileCheck %s --check-prefix=NO-KEY-INSTRUCTIONS
+// RUN: %clang %s -O0 -target x86_64 -gdwarf -gmlt -gkey-instructions -### 2>&1 | FileCheck %s --check-prefix=KEY-INSTRUCTIONS
+// RUN: %clang %s -O1 -target x86_64 -gdwarf -gmlt -### 2>&1 | FileCheck %s --check-prefix=KEY-INSTRUCTIONS
+// RUN: %clang %s -O1 -target x86_64 -gdwarf -gmlt -gkey-instructions -### 2>&1 | FileCheck %s --check-prefix=KEY-INSTRUCTIONS
+// RUN: %clang %s -O1 -target x86_64 -### 2>&1 | FileCheck %s --check-prefixes=NO-KEY-INSTRUCTIONS
+// RUN: %clang %s -O1 -target x86_64 -gcodeview -gmlt -### 2>&1 | FileCheck %s --check-prefixes=NO-KEY-INSTRUCTIONS
+//
+// RUN: %clang %s -target x86_64 -gdwarf -gmlt -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-OFF
+// RUN: %clang %s -target x86_64 -gdwarf -gmlt -gkey-instructions -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-ON
+// RUN: %clang %s -O0 -target x86_64 -gdwarf -gmlt -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-OFF
+// RUN: %clang %s -O0 -target x86_64 -gdwarf -gmlt -gkey-instructions -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-ON
+// RUN: %clang %s -O1 -target x86_64 -gdwarf -gmlt -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-ON
+// RUN: %clang %s -O1 -target x86_64 -gdwarf -gmlt -gkey-instructions -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-ON
+// RUN: %clang %s -O1 -target x86_64 -S -emit-llvm -o - | FileCheck %s --check-prefixes=SMOKETEST-OFF,SMOKETEST-NO-DEBUG
+// RUN: %clang %s -O1 -target x86_64 -gcodeview -gmlt -S -emit-llvm -o - | FileCheck %s --check-prefixes=SMOKETEST-OFF
+// SMOKETEST-NO-DEBUG: llvm.module.flags
+// SMOKETEST-NO-DEBUG-NOT: DICompileUnit
|
|
Ping (should I move the question to discourse?) |
|
A summary of DWARF file size/section size changes (how much change as a % of total file size in object and linked executables, as a % of aggregate DWARF section sizes in object and linked executables, and as a % of the specific DWARF section sizes (eg: {.rela,}.debug_line changes by x%) - with specific debug info flags (my preference would be for -fno-standalone-debug -gz=zstd with crel, but I understand if that's not your deployment target, etc, it's less relevant)) and which debuggers are effected in what ways (some discussion of gdb would be nice). If this has a cost and no benefit for lldb, seems like maybe we'd want to keep it off under lldb tuning. If it has a cost and no benefit to gdb - then I'd probably say off-by-default, on-for-SCE. |
|
Thanks for taking a look!
Here's bloaty figures for some CTMark builds ( I'm only including
There's a compile time cost to enabling the feature (0.52% on compile-time-tracker) which was included in the compile-time-cost tables in this post which explains where we put work in to "pay for" the cost of the feature (and we have longer term general improvements we'd like to make). I'm happy to get more numbers if there're any other configurations you'd like to see.
SCE (full disclosure - most tested) and GDB (*) and both work with the feature (both provide the promised improved optimisied-code stepping), but LLDB doesn't yet. I'd still like to gently suggest we enable it for LLDB too for uniformity, especially as we've tried to pay some of the costs in other areas, so the net "feature doesn't exist" -> "feature on by default" cost is < 0.3% increased (*) Checked with 15.0.50 and 12.0.90. GCC's |
|
Cool - thansks for all the details. I'm fine with making the change in general then, but should get some thougths from the apple folks, probably @JDevlieghere or @adrian-prantl |
|
I think you've all gone above and beyond to pay for the (marginal) cost increase from this feature, and we should go ahead and land this. Thanks for all the hard work and data gathering! |
|
Thanks all for the comments! In addition to the cc'd apple folks, @labath do you have an opinion on the topic of enabling this by default for all debuggers versus not doing so for LLDB tuning? (There's a question in the summary, and it's been discussed a little in the comments). One less exciting question that remains, if anyone has any ideas off the top of their heads:
|
|
Ideally, I'd also go with the uniform approach of enabling the feature everywhere. It's true that this doesn't have any value for lldb right now, but it also shouldn't break anything (lldb ignores Enabling the flag may also provide the incentive for someone to add support for this to lldb (I wish that someone was me, but it looks like that's not going to happen). Even if that happens a year from now, it would be nice if we could say that all binaries compiled within the last year would benefit from that. |
|
Thanks everyone for the discussion. It sounds like the consensus so far is to enable for all debugger tunings by default, for "plain" C/C++ if optimisations are enabled (though no apple folks have chimed in). I've added a check to filter out not-plain C/C++. Please could someone review the code and approve if they're happy? |
| bool IRInput = isLLVMIR(InputType); | ||
| bool PlainCOrCXX = isDerivedFromC(InputType) && !isCuda(InputType) && | ||
| !isHIP(InputType) && !isObjC(InputType) && | ||
| !isOpenCL(InputType); |
There was a problem hiding this comment.
Obvious question, is there a better way of selecting "just C and C++" rather than "list of extensions that this shouldn't apply to"?
There was a problem hiding this comment.
I don't love it either, but I couldn't find a better way. I asked for advice in the pull request description and a comment - I'll happily take suggestions if anyone has any! Are you happy for this to go in as-is?
Key Instructions (-gkey-instructions) is now enabled by default when DWARF is being emitted, the input is plain C/C++, and optimisations are enabled. Add release note for the change in default behaviour.
|
Hi Orlando, I wanted to share that we detected a significant (~0.4%) I looked at the code in this PR, and it has special handling for IR inputs, and I suspect that the reason is that we're using LTO, which has two compilation actions: One with C(++) source inputs, one with IR inputs, and I assume we need to pass I fully expect that key instructions will disturb the source locations in a way that increases source code drift, making old profiles from old binaries less accurate when rebuilding new binaries with key instructions enabled, so I expect a temporary regression. However, I think we will have to disable the feature internally temporarily to study this interaction further to confirm that the regression goes away after reprofiling. Other sample PGO users may have a similar experience, so I wanted to share ours. cc various AFDO stakeholders: @snehasish @amharc @alanzhao1 @mtrofin @asmok-g |
|
Hi Reid, thanks for the info!
The IR input change in this patch just shifts a check from caller to callee; that bit should be NFC. Unless I've designed it so that you can build an input with key instructions, and another without, then LTO them together and it should "just work"; scopes keep their key-instructions-ness regardless of inlining decisions. I don't think that's necessarily relevant to this problem you've run into, but thought it worth pointing out.
There's a hidden off switch
The intention is that key instructions should not change any line number associations, only is_stmt placement. i.e., if that's happening it is unexpected and a bug. It may change the layout of the table - an instruction that previously implicitly got a source location by virtue of sitting in an instruction range between two line table entries may now explicitly get a location. In other words, an instruction address may shift to being referenced in a line table entry where it was not previously and vice versa. That is just an encoding change rather than semantic one though. If this causes changes in PGO results it may be exposing a bug or assumption in the PGO tooling?
If you're able to share any additional info or reproducers I'm happy to try to help diagnose things. |
|
@rnk despite my claims in the comment above, with @omern1's help, I've been able to observe something like you've described with SPGO without LTO. Our initial experiments suggest that Key Instructions adds extra fields to |
6 participants
That's enabling Clang's -gkey-instructions, cc1's -gkey-instructions remains off by default.
Key Instructions improves the optimized-code debug-stepping experience in debuggers that use DWARF's
is_stmtline table register to determine stepping behaviour.The feature can be disabled with
-gno-key-instructions.RFC: https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668
Questions:
LLDB - I believe LLDB ignores Clang's
is_stmts for stepping, but I don't know whether movingis_stmts around in ways LLDB hasn't seen before (at least as output from Clang) is going to have any unexpected consequences. cc @adrian-prantl @JDevlieghere for opinions - what testing and checking would be needed / possible here, to be confident that this doesn't break optimized code debugging for LLDB?Clang languages/extensions - I've implemented support for C/C++, but I've not tested or specifically implemented anything for other languages (?) or extensions (?) like CUDA. Is there a good way to filter for this in the driver code in this patch?