-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add OwnedTargetMachine to manage llvm:TargetMachine #115911
Conversation
This comment has been minimized.
This comment has been minimized.
645dad2
to
ead21f6
Compare
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! have a few comments for small improvements, but overall this looks good and should improve the code.
use crate::{errors::LlvmError, llvm}; | ||
|
||
#[repr(transparent)] | ||
pub struct TargetMachineWrapper { |
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.
Not sure about the best name for this. I think TargetMachineWrapper
isn't the best name, but it's also not bad. TargetMachine
would be confusing. If you can think of something better, do rename it, but if you can't that's fine too and we can keep it.
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.
Some ideas might be
- TargetMachineBox (it kindo of behaves like a box, but its not copyable )
- TargetMachineLLVM (it the the for the associated type TargetMachine for the LLVM backend.
- TargetMachineOwned / OwnedTargetMachine
- UniqueTargetMachine / TargetMachineUnique
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.
Renamed it to TargetMachineBox. I think that fits what I am going for and should good give a good intuition of what it does. Also clarified this in the comments.
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 like OwnedTargetMachine
more than TargetMachineBox
, but TargetMachineBox
is fine too. If you want to, you can rename it again, or if you think it's fine and don't want to you can just use @bors r=Nilstrieb
and approve the PR.
Left some comments. Will be implementing those tomorrow. |
a30931c
to
22da275
Compare
Squashed commits @rustbot review |
r=me with or without another rename |
✌️ @nebulark, you can now approve this pull request! If @Nilstrieb told you to " |
22da275
to
034a39a
Compare
This comment has been minimized.
This comment has been minimized.
034a39a
to
148259c
Compare
instead of &'static mut and provides safe interface to create/dispose it.
148259c
to
3409ca6
Compare
☀️ Test successful - checks-actions |
Finished benchmarking commit (37390d6): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 632.214s -> 631.113s (-0.17%) |
LLVMRustDisposeTargetMachine taking a &mut could be undefined behaviour.
Wrapping it with a struct and using pointers instead avoids this problem.
In addition the TargetMachine is now automatically freed via the Wrappers drop impl. This should fix some memory leaks when
create_informational_target_machine was used, e.g.
rust/compiler/rustc_codegen_llvm/src/llvm_util.rs
Lines 291 to 314 in 327e6cf
r? @Nilstrieb