[Clang] Add "extend lifetime" flags and release note #110000
Conversation
SLTozer
requested review from
smeenai,
adrian-prantl,
AaronBallman,
jansvoboda11,
dwblaikie and
pogo59
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Stephen Tozer (SLTozer) ChangesFollowing the commit that added the fake use intrinsic to LLVM, this patch adds a pair of flags for the clang frontend that emit fake use intrinsics, for the purpose of extending the lifetime of variables (either all source variables, or just the Full diff: https://github.com/llvm/llvm-project/pull/110000.diff 7 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 14907e7db18de3..249f2171babda1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -211,6 +211,16 @@ New Compiler Flags
only for thread-local variables, and none (which corresponds to the
existing ``-fno-c++-static-destructors`` flag) skips all static
destructors registration.
+- The ``-fextend-lifetimes`` and ``-fextend-this-ptr`` flags have been added to
+ allow for improved debugging of optimized code. Using ``-fextend-lifetimes``
+ will cause Clang to generate code that tries to preserve the lifetimes of
+ source variables, meaning that variables will typically be visible in a
+ debugger more often. The ``-fextend-this-ptr`` flag has the same behaviour,
+ but applies only to the ``this`` variable in C++ class member functions. Note
+ that this flag modifies the optimizations that Clang performs, which will
+ result in reduced performance in generated code; however, this feature will
+ not extend the lifetime of some variables in cases where doing so would have
+ too severe an impact on generated code performance.
Deprecated Compiler Flags
-------------------------
diff --git a/clang/include/clang/Basic/CodeGenOptions.def b/clang/include/clang/Basic/CodeGenOptions.def
index 2893377e5a38be..18f5225813133f 100644
--- a/clang/include/clang/Basic/CodeGenOptions.def
+++ b/clang/include/clang/Basic/CodeGenOptions.def
@@ -388,6 +388,12 @@ CODEGENOPT(EnableTLSDESC, 1, 0)
/// Bit size of immediate TLS offsets (0 == use the default).
VALUE_CODEGENOPT(TLSSize, 8, 0)
+/// Whether to extend the live range of the `this` pointer.
+CODEGENOPT(ExtendThisPtr, 1, 0)
+
+/// Whether to extend the live ranges of all local variables.
+CODEGENOPT(ExtendLifetimes, 1, 0)
+
/// The default stack protector guard offset to use.
VALUE_CODEGENOPT(StackProtectorGuardOffset, 32, INT_MAX)
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 23bd686a85f526..a82dbbc113d105 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -4263,6 +4263,15 @@ def stack_usage_file : Separate<["-"], "stack-usage-file">,
Visibility<[CC1Option]>,
HelpText<"Filename (or -) to write stack usage output to">,
MarshallingInfoString<CodeGenOpts<"StackUsageOutput">>;
+def fextend_this_ptr : Flag <["-"], "fextend-this-ptr">, Group<f_Group>,
+ MarshallingInfoFlag<CodeGenOpts<"ExtendThisPtr">>,
+ HelpText<"Extend the lifetime of the 'this' pointer to improve visibility "
+ "in optimized debugging">, Visibility<[ClangOption, CC1Option]>;
+def fextend_lifetimes : Flag <["-"], "fextend-lifetimes">, Group<f_Group>,
+ MarshallingInfoFlag<CodeGenOpts<"ExtendLifetimes">>,
+ HelpText<"Extend the lifetimes of local variables and parameters to improve "
+ "visibility in optimized debugging">,
+ Visibility<[ClangOption, CC1Option]>;
defm unique_basic_block_section_names : BoolFOption<"unique-basic-block-section-names",
CodeGenOpts<"UniqueBasicBlockSectionNames">, DefaultFalse,
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 4ae981e4013e9c..2453c580f7db0f 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -2543,6 +2543,10 @@ void CodeGenModule::ConstructAttributeList(StringRef Name,
if (shouldDisableTailCalls())
FuncAttrs.addAttribute("disable-tail-calls", "true");
+ // Mark the function as having fake uses when -fextend-lifetimes is on.
+ if (CodeGenOpts.ExtendLifetimes || CodeGenOpts.ExtendThisPtr)
+ FuncAttrs.addAttribute(llvm::Attribute::HasFakeUses);
+
// CPU/feature overrides. addDefaultFunctionDefinitionAttributes
// handles these separately to set them based on the global defaults.
GetCPUAndFeaturesAttributes(CalleeInfo.getCalleeDecl(), FuncAttrs);
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 0bab48caf1a5e2..3fe7817acf61c2 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -7583,6 +7583,11 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
if (Args.hasArg(options::OPT_fretain_comments_from_system_headers))
CmdArgs.push_back("-fretain-comments-from-system-headers");
+ if (Args.hasArg(options::OPT_fextend_this_ptr))
+ CmdArgs.push_back("-fextend-this-ptr");
+ if (Args.hasArg(options::OPT_fextend_lifetimes))
+ CmdArgs.push_back("-fextend-lifetimes");
+
// Forward -fcomment-block-commands to -cc1.
Args.AddAllArgs(CmdArgs, options::OPT_fcomment_block_commands);
// Forward -fparse-all-comments to -cc1.
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index efd852593468aa..93972c37ac6821 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -2217,6 +2217,11 @@ bool CompilerInvocation::ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args,
Args.getAllArgValues(OPT_fsanitize_trap_EQ), Diags,
Opts.SanitizeTrap);
+ Opts.ExtendThisPtr =
+ Opts.OptimizationLevel > 0 && Args.hasArg(OPT_fextend_this_ptr);
+ Opts.ExtendLifetimes =
+ Opts.OptimizationLevel > 0 && Args.hasArg(OPT_fextend_lifetimes);
+
Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
if (!LangOpts->CUDAIsDevice)
diff --git a/clang/test/CodeGen/extend-lifetimes-hasfakeuses.c b/clang/test/CodeGen/extend-lifetimes-hasfakeuses.c
new file mode 100644
index 00000000000000..081bace3dc7cce
--- /dev/null
+++ b/clang/test/CodeGen/extend-lifetimes-hasfakeuses.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 %s -emit-llvm -O2 -fextend-lifetimes -o - | FileCheck --check-prefixes=CHECK-ALL,CHECK-O2 %s
+// RUN: %clang_cc1 %s -emit-llvm -O0 -fextend-lifetimes -o - | FileCheck --check-prefixes=CHECK-ALL,CHECK-O0 %s
+
+// Checks that we emit the function attribute has_fake_uses when
+// -fextend-lifetimes is on and optimizations are enabled, and that it does not
+// when optimizations are disabled.
+
+// CHECK-ALL: define {{.*}}void @foo
+// CHECK-O2: attributes #0 = {{{.*}}has_fake_uses
+// CHECK-O0-NOT: attributes #0 = {{{.*}}has_fake_uses
+
+void foo() {}
|
|
@llvm/pr-subscribers-clang-driver Author: Stephen Tozer (SLTozer) ChangesFollowing the commit that added the fake use intrinsic to LLVM, this patch adds a pair of flags for the clang frontend that emit fake use intrinsics, for the purpose of extending the lifetime of variables (either all source variables, or just the Full diff: https://github.com/llvm/llvm-project/pull/110000.diff 7 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 14907e7db18de3..249f2171babda1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -211,6 +211,16 @@ New Compiler Flags
only for thread-local variables, and none (which corresponds to the
existing ``-fno-c++-static-destructors`` flag) skips all static
destructors registration.
+- The ``-fextend-lifetimes`` and ``-fextend-this-ptr`` flags have been added to
+ allow for improved debugging of optimized code. Using ``-fextend-lifetimes``
+ will cause Clang to generate code that tries to preserve the lifetimes of
+ source variables, meaning that variables will typically be visible in a
+ debugger more often. The ``-fextend-this-ptr`` flag has the same behaviour,
+ but applies only to the ``this`` variable in C++ class member functions. Note
+ that this flag modifies the optimizations that Clang performs, which will
+ result in reduced performance in generated code; however, this feature will
+ not extend the lifetime of some variables in cases where doing so would have
+ too severe an impact on generated code performance.
Deprecated Compiler Flags
-------------------------
diff --git a/clang/include/clang/Basic/CodeGenOptions.def b/clang/include/clang/Basic/CodeGenOptions.def
index 2893377e5a38be..18f5225813133f 100644
--- a/clang/include/clang/Basic/CodeGenOptions.def
+++ b/clang/include/clang/Basic/CodeGenOptions.def
@@ -388,6 +388,12 @@ CODEGENOPT(EnableTLSDESC, 1, 0)
/// Bit size of immediate TLS offsets (0 == use the default).
VALUE_CODEGENOPT(TLSSize, 8, 0)
+/// Whether to extend the live range of the `this` pointer.
+CODEGENOPT(ExtendThisPtr, 1, 0)
+
+/// Whether to extend the live ranges of all local variables.
+CODEGENOPT(ExtendLifetimes, 1, 0)
+
/// The default stack protector guard offset to use.
VALUE_CODEGENOPT(StackProtectorGuardOffset, 32, INT_MAX)
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 23bd686a85f526..a82dbbc113d105 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -4263,6 +4263,15 @@ def stack_usage_file : Separate<["-"], "stack-usage-file">,
Visibility<[CC1Option]>,
HelpText<"Filename (or -) to write stack usage output to">,
MarshallingInfoString<CodeGenOpts<"StackUsageOutput">>;
+def fextend_this_ptr : Flag <["-"], "fextend-this-ptr">, Group<f_Group>,
+ MarshallingInfoFlag<CodeGenOpts<"ExtendThisPtr">>,
+ HelpText<"Extend the lifetime of the 'this' pointer to improve visibility "
+ "in optimized debugging">, Visibility<[ClangOption, CC1Option]>;
+def fextend_lifetimes : Flag <["-"], "fextend-lifetimes">, Group<f_Group>,
+ MarshallingInfoFlag<CodeGenOpts<"ExtendLifetimes">>,
+ HelpText<"Extend the lifetimes of local variables and parameters to improve "
+ "visibility in optimized debugging">,
+ Visibility<[ClangOption, CC1Option]>;
defm unique_basic_block_section_names : BoolFOption<"unique-basic-block-section-names",
CodeGenOpts<"UniqueBasicBlockSectionNames">, DefaultFalse,
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 4ae981e4013e9c..2453c580f7db0f 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -2543,6 +2543,10 @@ void CodeGenModule::ConstructAttributeList(StringRef Name,
if (shouldDisableTailCalls())
FuncAttrs.addAttribute("disable-tail-calls", "true");
+ // Mark the function as having fake uses when -fextend-lifetimes is on.
+ if (CodeGenOpts.ExtendLifetimes || CodeGenOpts.ExtendThisPtr)
+ FuncAttrs.addAttribute(llvm::Attribute::HasFakeUses);
+
// CPU/feature overrides. addDefaultFunctionDefinitionAttributes
// handles these separately to set them based on the global defaults.
GetCPUAndFeaturesAttributes(CalleeInfo.getCalleeDecl(), FuncAttrs);
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 0bab48caf1a5e2..3fe7817acf61c2 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -7583,6 +7583,11 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
if (Args.hasArg(options::OPT_fretain_comments_from_system_headers))
CmdArgs.push_back("-fretain-comments-from-system-headers");
+ if (Args.hasArg(options::OPT_fextend_this_ptr))
+ CmdArgs.push_back("-fextend-this-ptr");
+ if (Args.hasArg(options::OPT_fextend_lifetimes))
+ CmdArgs.push_back("-fextend-lifetimes");
+
// Forward -fcomment-block-commands to -cc1.
Args.AddAllArgs(CmdArgs, options::OPT_fcomment_block_commands);
// Forward -fparse-all-comments to -cc1.
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index efd852593468aa..93972c37ac6821 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -2217,6 +2217,11 @@ bool CompilerInvocation::ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args,
Args.getAllArgValues(OPT_fsanitize_trap_EQ), Diags,
Opts.SanitizeTrap);
+ Opts.ExtendThisPtr =
+ Opts.OptimizationLevel > 0 && Args.hasArg(OPT_fextend_this_ptr);
+ Opts.ExtendLifetimes =
+ Opts.OptimizationLevel > 0 && Args.hasArg(OPT_fextend_lifetimes);
+
Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
if (!LangOpts->CUDAIsDevice)
diff --git a/clang/test/CodeGen/extend-lifetimes-hasfakeuses.c b/clang/test/CodeGen/extend-lifetimes-hasfakeuses.c
new file mode 100644
index 00000000000000..081bace3dc7cce
--- /dev/null
+++ b/clang/test/CodeGen/extend-lifetimes-hasfakeuses.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 %s -emit-llvm -O2 -fextend-lifetimes -o - | FileCheck --check-prefixes=CHECK-ALL,CHECK-O2 %s
+// RUN: %clang_cc1 %s -emit-llvm -O0 -fextend-lifetimes -o - | FileCheck --check-prefixes=CHECK-ALL,CHECK-O0 %s
+
+// Checks that we emit the function attribute has_fake_uses when
+// -fextend-lifetimes is on and optimizations are enabled, and that it does not
+// when optimizations are disabled.
+
+// CHECK-ALL: define {{.*}}void @foo
+// CHECK-O2: attributes #0 = {{{.*}}has_fake_uses
+// CHECK-O0-NOT: attributes #0 = {{{.*}}has_fake_uses
+
+void foo() {}
|
|
Ping - if anyone isn't able to review themselves but has an idea of someone who might, adding/pinging them would also be appreciated. |
There was a problem hiding this comment.
It seems a little odd to have only positive forms of these flags; usually toggles would have both positive and negative forms. Maybe @MaskRay has an opinion?
The release note doesn't say: Does -fextend-lifetimes imply -fextend-this-pointer? They're implemented as independent toggles but the effect isn't really independent IIRC. I wonder (years after it was originally implemented downstream, I know) whether we'd be better off with -fextend-lifetimes[={all,this,none}]. (But maybe wait for a second opinion.)
| @@ -2217,6 +2217,11 @@ bool CompilerInvocation::ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, | |||
| Args.getAllArgValues(OPT_fsanitize_trap_EQ), Diags, | |||
| Opts.SanitizeTrap); | |||
|
|
|||
| Opts.ExtendThisPtr = | |||
| Opts.OptimizationLevel > 0 && Args.hasArg(OPT_fextend_this_ptr); | |||
There was a problem hiding this comment.
Is it worth maybe making this inconditional, even at -O0, in case someone's put an optimize attribute on a function, etc, and wants to override it?
There was a problem hiding this comment.
I'm currently looking into this to see if there would be any notable negative consequences, but for now I'm at least happy to move this to the next patch along: in that patch, we emit fake uses for functions when OptimizationLevel != 0 and or we've passed -disable-O0-optnone - that at least gives the ability to pass -O0 -fextend-lifetimes together if desired.
| // when optimizations are disabled. | ||
|
|
||
| // CHECK-ALL: define {{.*}}void @foo | ||
| // CHECK-O2: attributes #0 = {{{.*}}has_fake_uses |
There was a problem hiding this comment.
Interesting model - I haven't reviewed the other patches in this direction, but I'm a bit surprised this is the only IR effect from the frontend.
I take it this attribute is added, which then causes some middle end pass to add the fake uses themselves?
That seems a bit quirky in terms of creating an attribute to communicate that - I guess other directions have already been considered/found unsuitable? (a couple of others that come to mind include: having the frontend create the fake uses (limits reusability for other front ends) or having the frontend choose to insert the fake use pass or not (and then the pass is unconditional/doesn't look for an attribute) - I /think/ a combination of these might be how the sanitizers work? (frontend chooses to include certain passes but might also have to do some different IRGen too))
There was a problem hiding this comment.
This isn't the only effect from the front end; this flag will actually generate llvm.fake.use intrinsics, but only for source variables, which this test does not have. But as you've noted, this test should probably include variables to demonstrate that we also don't emit fake uses for those at O0!
Correct, this is sort of implied by |
|
This patch has been changed as a result of some other patches that have removed the need for the attribute that this patch once added. Since this patch does not emit fake uses (the actual emission of fake uses is added in the next patch), that leaves this patch in the awkward position of adding flags and a release note but not actually doing anything. I'd prefer to leave this patch separate though, because the next patch is already tricky to review - adding more to that patch would probably slow things down a bit. |
|
The additions here look fine; however I think there's generally precedent that anything we add needs to have /some/ kind of test, or be covered by something already existing, otherwise we're vulnerable to:
As far as I understand it, these are driver options that will be passed through to cc1? In that case we can at least test the passthrough, i.e. that |
Following the commit that added the fake use intrinsic to LLVM, this patch adds a pair of flags for the clang frontend that emit fake use intrinsics, for the purpose of extending the lifetime of variables (either all source variables, or just the `this` pointer). This patch does not implement the fake use intrinsic emission of the flags themselves, it simply adds the flags, the corresponding release note, and the attachment of the `has_fake_uses` attribute to affected functions; the remaining functionality appears in the next patch.
I think my preferred approach would be to merge both patches simultaneously, or else roll this patch into the next one - we have some tests that use |
There was a problem hiding this comment.
I think this flag name is going to be extremely confusing. "lifetime" is a notion defined formally by the C++ language standard, and "lifetime extension" is again a process defined in C++ with a specific meaning. Those meanings have nothing to do with what these flags control.
Please consider alternative names for this flag that make it clear that it's not talking about changing language semantics, just how code is lowered. Maybe -fextend-live-ranges or -fkeep-alive or something like that? (Maybe even something that mentions debugging as the intended use case, as opposed to, say, supporting stack-walking GC?)
|
Looks like the tests in #110102 are named |
SLTozer
force-pushed
the
add-extend-lifetimes-flag
branch
from
2957d15 to
984e050
Compare
|
I've tried renaming the option to |
|
I've added a test for this patch that tests the driver - since we will now be relying on the driver to decide whether to pass these flags to the frontend, rather than trivially always doing so (so that later on we can use the driver to enable this at |
|
Post-holiday ping - I believe all the comments have been addressed now, does the new form look acceptable? |
clang/docs/ReleaseNotes.rst
Outdated
| through optimizations, meaning that variables will typically be visible in a | ||
| debugger more often. The flag has two levels: ``-fextend-variable-liveness``, | ||
| or ``-fextend-variable-liveness=all``, extendes the liveness of all user | ||
| variables and the ``this`` pointer. Alternatively ``-fextend-this-ptr``, or |
There was a problem hiding this comment.
Looks like the flag name supported is -fextend-this-ptr-liveness, not -fextend-this-ptr.
I also wonder whether the -fextend-this-ptr-liveness flag carries its weight -- we've historically removed these special-case flags in favor of the more general things, as we did with -fsanitize= for example. I understand that this is being upstreamed from some existing deployment; does the alias help there? Having it around for a shorter time period with an intent to eventually remove it might be reasonable.
There was a problem hiding this comment.
Sounds reasonable to me - the this variant is quite useful for more performance-sensitive cases, but it doesn't need to exist under a separate flag name; changed it.
Following the commit that added the fake use intrinsic to LLVM, this patch adds a pair of flags for the clang frontend that emit fake use intrinsics, for the purpose of extending the lifetime of variables (either all source variables, or just the `this` pointer). This patch does not implement the fake use intrinsic emission of the flags themselves, it simply adds the flags, the corresponding release note, and the attachment of the `has_fake_uses` attribute to affected functions; the remaining functionality appears in the next patch. Co-authored-by: Stephen Tozer <[email protected]>
|
Merged in 71ab44a8, via command line so as to provide proper commit author attribution to @wolfy1961 (as this doesn't seem to be possible via the web interface). |
…-liveness This patch contains a number of changes relating to the above flag; primarily it updates references to the old flag names, "-fextend-lifetimes" and "-fextend-this-ptr" to refer to the new names. These changes are all NFC. This patch also removes the explicit -fextend-this-ptr-liveness flag alias, and shortens the help-text for the main flag; these are both changes that were meant to be applied in the initial review (llvm#110000), but due to some user-error on my part they were not included in the merged commit.
…ess (#124767) This patch contains a number of changes relating to the above flag; primarily it updates comment references to the old flag names, "-fextend-lifetimes" and "-fextend-this-ptr" to refer to the new names, "-fextend-variable-liveness[={all,this}]". These changes are all NFC. This patch also removes the explicit -fextend-this-ptr-liveness flag alias, and shortens the help-text for the main flag; these are both changes that were meant to be applied in the initial PR (#110000), but due to some user-error on my part they were not included in the merged commit.
Following the commit that added the fake use intrinsic to LLVM, this patch adds a pair of flags for the clang frontend that emit fake use intrinsics, for the purpose of extending the lifetime of variables (either all source variables, or just the
thispointer). This patch does not implement the fake use intrinsic emission of the flags themselves, it simply adds the flags, the corresponding release note, and the attachment of thehas_fake_usesattribute to affected functions; the remaining functionality appears in the next patch.All code originally written by @wolfy1961, while I'll be handling the review.