[DebugInfo][RemoveDIs] Add flag to use "new" debug-info in opt #71937
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The option to turn on the non-intrinsic form of debug-info currently requires that LLVM is built with the LLVM_EXPERIMENTAL_DEBUGINFO_ITERATORS cmake flag enabled, so that some (slight) performance regressions aren't on-by-default during the prototype/testing period. However, we still want to be able to _optionally_ run tests, if support is built into LLVM. Hence adding this flag, --try-experimental-debuginfo-iterators. This turns the --experimental-debuginfo-iterators flag on if support is built in, or leaves it off. This means we can run tests that: * Use normal dbg.value intrinsics if there's no support, or * Uses non-instruction DPValues if there is support. Which means we can start getting test coverage of DPValues/RemoveDIs behaviour, from in-tree tests, on our RemoveDIs buildbot.
|
@llvm/pr-subscribers-debuginfo Author: Jeremy Morse (jmorse) ChangesOur option to turn on the non-intrinsic form of debug-info ( To allow optionally exercising the non-intrinsic debug-info code, this patch adds
Which means we can start getting test coverage of DPValues/RemoveDIs behaviour, from in-tree tests, on our RemoveDIs buildbot. All the code to do with automagically converting from one form to the other landed in 10a9e74. Full diff: https://github.com/llvm/llvm-project/pull/71937.diff 1 Files Affected:
diff --git a/llvm/tools/opt/opt.cpp b/llvm/tools/opt/opt.cpp
index bb6627364442ef7..129f754eedda71d 100644
--- a/llvm/tools/opt/opt.cpp
+++ b/llvm/tools/opt/opt.cpp
@@ -279,6 +279,12 @@ static cl::list<std::string>
PassPlugins("load-pass-plugin",
cl::desc("Load passes from plugin library"));
+static cl::opt<bool> TryUseNewDbgInfoFormat("try-experimental-debuginfo-iterators",
+ cl::desc("Enable debuginfo iterator positions, if they're built in"),
+ cl::init(false));
+
+extern cl::opt<bool> UseNewDbgInfoFormat;
+
//===----------------------------------------------------------------------===//
// CodeGen-related helper functions.
//
@@ -438,6 +444,19 @@ int main(int argc, char **argv) {
initializeReplaceWithVeclibLegacyPass(Registry);
initializeJMCInstrumenterPass(Registry);
+ // RemoveDIs debug-info transition: tests may request that we /try/ to use the
+ // new debug-info format, if it's built in.
+ if (TryUseNewDbgInfoFormat) {
+#ifdef EXPERIMENTAL_DEBUGINFO_ITERATORS
+ // If LLVM was built with support for this, turn the new debug-info format
+ // on.
+ UseNewDbgInfoFormat = true;
+#else
+ // It it wasn't, do nothing.
+ ;
+#endif
+ }
+
SmallVector<PassPlugin, 1> PluginList;
PassPlugins.setCallback([&](const std::string &PluginPath) {
auto Plugin = PassPlugin::Load(PluginPath);
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
OCHyams
reviewed
Nov 10, 2023
llvm/tools/opt/opt.cpp
Outdated
Comment on lines
447
to
458
| // RemoveDIs debug-info transition: tests may request that we /try/ to use the | ||
| // new debug-info format, if it's built in. | ||
| if (TryUseNewDbgInfoFormat) { | ||
| #ifdef EXPERIMENTAL_DEBUGINFO_ITERATORS | ||
| // If LLVM was built with support for this, turn the new debug-info format | ||
| // on. | ||
| UseNewDbgInfoFormat = true; | ||
| #else | ||
| // It it wasn't, do nothing. | ||
| ; | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Suggested change
| // RemoveDIs debug-info transition: tests may request that we /try/ to use the | |
| // new debug-info format, if it's built in. | |
| if (TryUseNewDbgInfoFormat) { | |
| #ifdef EXPERIMENTAL_DEBUGINFO_ITERATORS | |
| // If LLVM was built with support for this, turn the new debug-info format | |
| // on. | |
| UseNewDbgInfoFormat = true; | |
| #else | |
| // It it wasn't, do nothing. | |
| ; | |
| #endif | |
| } | |
| // RemoveDIs debug-info transition: tests may request that we /try/ to use the | |
| // new debug-info format, if it's built in. | |
| #ifdef EXPERIMENTAL_DEBUGINFO_ITERATORS | |
| if (TryUseNewDbgInfoFormat) | |
| UseNewDbgInfoFormat = true; | |
| #endif | |
| } |
There was a problem hiding this comment.
Unless you wanted to add prints etc to the false branch, I don't think we need it?
There was a problem hiding this comment.
@OCHyams might need a (void)TryUseNewDbgInfoFormat; outside the ifdef, to suppress an unused-variable warning in the iterators-off mode.
OCHyams
approved these changes
Nov 10, 2023
llvm/tools/opt/opt.cpp
Outdated
Comment on lines
447
to
458
| // RemoveDIs debug-info transition: tests may request that we /try/ to use the | ||
| // new debug-info format, if it's built in. | ||
| if (TryUseNewDbgInfoFormat) { | ||
| #ifdef EXPERIMENTAL_DEBUGINFO_ITERATORS | ||
| // If LLVM was built with support for this, turn the new debug-info format | ||
| // on. | ||
| UseNewDbgInfoFormat = true; | ||
| #else | ||
| // It it wasn't, do nothing. | ||
| ; | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Unless you wanted to add prints etc to the false branch, I don't think we need it?
|
Pushed up some commits to simplify the diff, and also uh, test the flag after the command line options have actually been parsed (it's always-false otherwise). plus clang-format. |
jmorse
added a commit
to jmorse/llvm-project
that referenced
this pull request
Nov 14, 2023
DPValues are the non-intrinsic replacements for dbg.values, and when an IR
function is converted by SelectionDAG we need to convert the variable
location information in the same way. Happily all the information is in the
same format, it's just stored in a slightly different object, therefore
this patch refactors a few things to store the set of
{Variable,Expr,DILocation,Location} instead of just a pointer to a
DbgValueInst.
This also adds a hook in llc that's much like the one I've added to opt in
PR llvm#71937, allowing tests to optionally ask for the use RemoveDIs mode if
support for it is built into the compiler.
I've added that flag to a variety of SelectionDAG debug-info tests to
ensure that we get some coverage on the RemoveDIs / debug-info-iterator
buildbot.
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
…71937) Our option to turn on the non-intrinsic form of debug-info (`--experimental-debuginfo-iterators`) currently requires that LLVM is built with the `LLVM_EXPERIMENTAL_DEBUGINFO_ITERATORS` cmake flag enabled, so that some (slight) performance regressions aren't on-by-default during the prototype/testing period. However, we still want to be able to _optionally_ run tests, if support is built into LLVM. To allow optionally exercising the non-intrinsic debug-info code, this patch adds `--try-experimental-debuginfo-iterators` to opt, which turns the `--experimental-debuginfo-iterators` flag on if support is built in, or leaves it off. This means we can run tests that: * Use normal dbg.value intrinsics if there's no support, or * Uses non-instruction DPValues if there is support. Which means we can start getting test coverage of DPValues/RemoveDIs behaviour, from in-tree tests, on our RemoveDIs buildbot. All the code to do with automagically converting from one form to the other landed in 10a9e74.
jmorse
added a commit
that referenced
this pull request
Nov 21, 2023
) DPValues are the non-intrinsic replacements for dbg.values, and when an IR function is converted by SelectionDAG we need to convert the variable location information in the same way. Happily all the information is in the same format, it's just stored in a slightly different object, therefore this patch refactors a few things to store the set of {Variable,Expr,DILocation,Location} instead of just a pointer to a DbgValueInst. This also adds a hook in llc that's much like the one I've added to opt in PR #71937, allowing tests to optionally ask for the use RemoveDIs mode if support for it is built into the compiler. I've added that flag to a variety of SelectionDAG debug-info tests to ensure that we get some coverage on the RemoveDIs / debug-info-iterator buildbot.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our option to turn on the non-intrinsic form of debug-info (
--experimental-debuginfo-iterators) currently requires that LLVM is built with theLLVM_EXPERIMENTAL_DEBUGINFO_ITERATORScmake flag enabled, so that some (slight) performance regressions aren't on-by-default during the prototype/testing period. However, we still want to be able to optionally run tests, if support is built into LLVM.To allow optionally exercising the non-intrinsic debug-info code, this patch adds
--try-experimental-debuginfo-iteratorsto opt, which turns the--experimental-debuginfo-iteratorsflag on if support is built in, or leaves it off. This means we can run tests that:Which means we can start getting test coverage of DPValues/RemoveDIs behaviour, from in-tree tests, on our RemoveDIs buildbot. All the code to do with automagically converting from one form to the other landed in 10a9e74.