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.
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
Introduce
-Zvirtual-function-elimination
codegen flag #96285Introduce
-Zvirtual-function-elimination
codegen flag #96285Changes from all commits
def3fd8
20f597f
d55787a
e1c1d0f
e96e6e2
996c6b7
a93ea7e
195f208
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 this be 11?
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 believe the
HasModuleFlag
should be checking for1
, but also adding this flag here seems super sketchy to me. This is a pretty internal thing to LTO AFAICT.In that context, it would be great if this code had a comment with context as to why this is being done.
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.
In the mono repo, I found the value 1.
The LTOGenerator only adds the flag.
https://github.com/llvm/llvm-project/blob/09c2b7c35af8c4bad39f03e9f60df8bd07323028/llvm/lib/LTO/LTOCodeGenerator.cpp#L541
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.
The code here is based on the file tschuett linked above. In the LLVM repo that code is responsible to set up LTO. Adding this flag was missing from the Rust code that should emulate the code in
LTOCodeGenerator.cpp
.The
11
argument in theHasModuleFlag
is the size of the string passed to this function, not the value of the flag. The1
argument in theAddModuleFlag
function is the value of the module flag. LLVM doesn't need a string size here, because it expects a\0
terminated string in this function.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 that the closest existing concept similar to
vcall_visibility
would be reachability, rather than visibility.Example with an incorrectly optimized vtables showing why visibility is insufficient
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.
Thanks for pointing this out. I don't really have a solution for this right now. I tried looking at the reachability of the
trait_ref
and the members of the traits with theprivacy_access_levels
query. But for all of the traits and the functions in the traits (queried withtcx.vtable_entries
) this just returnsNone
, meaning it is not reachable IIUC.I guess I would somehow have to check the reachability for the
dyn Trait
types. But I couldn't figure out how to do that.Do you have ideas on how to best address this issue?