-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[LTO] Reset DiscardValueNames in optimize(). #78705
Conversation
@llvm/pr-subscribers-lto Author: Florian Hahn (fhahn) ChangeslibLTO parses options late, so at the moment the option is ignored. To fix that, Note that we keep the value name of I tried to improve this in libLTO, but I am not sure if there's a suitable callback when all options have been set. Full diff: https://github.com/llvm/llvm-project/pull/78705.diff 2 Files Affected:
diff --git a/llvm/lib/LTO/LTOCodeGenerator.cpp b/llvm/lib/LTO/LTOCodeGenerator.cpp
index 52d8fff14be9cec..727ab7b0db41a6d 100644
--- a/llvm/lib/LTO/LTOCodeGenerator.cpp
+++ b/llvm/lib/LTO/LTOCodeGenerator.cpp
@@ -582,6 +582,9 @@ bool LTOCodeGenerator::optimize() {
if (!this->determineTarget())
return false;
+ // libLTO parses options late, so re-set them here.
+ Context.setDiscardValueNames(LTODiscardValueNames);
+
auto DiagFileOrErr = lto::setupLLVMOptimizationRemarks(
Context, RemarksFilename, RemarksPasses, RemarksFormat,
RemarksWithHotness, RemarksHotnessThreshold);
diff --git a/llvm/test/tools/lto/discard-value-names.ll b/llvm/test/tools/lto/discard-value-names.ll
index 723b0701ae22cee..04d25eaf6067c54 100644
--- a/llvm/test/tools/lto/discard-value-names.ll
+++ b/llvm/test/tools/lto/discard-value-names.ll
@@ -7,11 +7,10 @@
; The test requires asserts, as it depends on the default value for
; -lto-discard-value-names at the moment.
-; FIXME: -lto-discard-value-names is ignored at the moment.
; REQUIRES: asserts
-; DISCARD: %cmp.i = icmp
+; DISCARD: %{{[0-9]+}} = icmp
; DISCARD: %add = add i32
; KEEP: %cmp.i = icmp
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the flag doesn't work because .bc
is save-temp
outputs that are not real outputs from libLTO but only for debugging intermediates. I am not against the quality of life improvement here.
Is this a FullLTO problem only or it affects thinLTO as well?
@@ -7,11 +7,10 @@ | |||
|
|||
; The test requires asserts, as it depends on the default value for | |||
; -lto-discard-value-names at the moment. | |||
; FIXME: -lto-discard-value-names is ignored at the moment. | |||
|
|||
; REQUIRES: asserts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the REQUIRED to be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is needed as otherwise the default is to discard value names. Left as is for now
libLTO parses options late, so re-set them in optimize(). When LTOCodeGenerator's constructor executes, the options haven't been passed by the linker to libLTO yet. Note that we keep the value name of `%add = add..` because when the module is imported, DiscardValueNames is still set to false (the default when building wiht assertions). I tried to improve this in libLTO, but I am not sure if there's a suitable callback when all options have been set.
96f162a
to
e13d023
Compare
libLTO parses options late, so at the moment the option is ignored. To fix that,
re-set it in optimize(), as at this point the options have been parsed. When
LTOCodeGenerator's constructor executes, the options haven't been passed
by the linker to libLTO yet.
Note that we keep the value name of
%add = add..
because when the moduleis imported, DiscardValueNames is still set to false (the default when building
with assertions).
I tried to improve this in libLTO, but I am not sure if there's a suitable callback when all options have been set.