Remove unnecessary LLVMRustPersonalityFn binding #39319
Conversation
|
Since I’m on battery currently, didn’t build to check if it works yet. |
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
nagisa
force-pushed
the
remove-rustsetpersonalityfn
branch
from
19a2ab3 to
20738b6
Compare
| @@ -1105,12 +1105,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { | |||
| } | |||
| } | |||
|
|
|||
| pub fn set_personality_fn(&self, personality: ValueRef) { | |||
There was a problem hiding this comment.
Could we keep this but change it to call llvm::LLVMSetPersonalityFn(self.llfn(), llpersonality); instead of inlining it? That would keep the unsafe more local to builder.rs which is a win, I believe.
Granted this does save us one builder to function transition, but I don't think (hope) that's an expensive operation.
LLVM Core C bindings provide this function for all the versions back to what we support (3.7), and helps to avoid this unnecessary builder->function transition every time. Also a negative diff.
nagisa
force-pushed
the
remove-rustsetpersonalityfn
branch
from
20738b6 to
1363cda
Compare
| bcx.set_personality_fn(mircx.ccx.eh_personality()); | ||
| unsafe { | ||
| llvm::LLVMSetPersonalityFn(mircx.llfn, mircx.ccx.eh_personality()); | ||
| } |
There was a problem hiding this comment.
... am I right in inferring from this change that bcx.llfn() is not the same as mircx.llfn? It seems surprising.
And If that is actually true... it probably could use a comment.
There was a problem hiding this comment.
It is the same. It just avoids going through LLVM to get the information that’s already available here.
There was a problem hiding this comment.
Okay. I would personally have left it alone, but I don't care enough to block this PR.
|
@bors r+ rollup |
|
📌 Commit 1363cda has been approved by |
… r=pnkfelix Remove unnecessary LLVMRustPersonalityFn binding LLVM Core C bindings provide this function for all the versions back to what we support (3.7), and helps to avoid this unnecessary builder->function transition every time. Also a negative diff. Fixes rust-lang#38462 (although it was pretty much fixed already)
LLVM Core C bindings provide this function for all the versions back to what we support (3.7), and
helps to avoid this unnecessary builder->function transition every time. Also a negative diff.
Fixes #38462 (although it was pretty much fixed already)