Add an InstCombine for redundant casts #108246
Conversation
|
r? @eholk (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
saethlin
force-pushed
the
instcombine-redundant-casts
branch
from
5b35035
to
13df8f3
Compare
tests/mir-opt/casts.rs
Outdated
| pub fn redundant(x: *const u8) -> *const u8 { | ||
| x as *const u8 | ||
| } |
There was a problem hiding this comment.
Is this testing anything? Rust playground is telling me that this hasn't changed anything from the MIR that gets emitted for redundant on stable.
There was a problem hiding this comment.
Sure wasn't testing anything. I wish we had a better way to test stuff like this. The easy way out is to look at the pass's diff, but I don't care care very much what this particular pass does, only that the pattern gets optimized away.
There was a problem hiding this comment.
We should definitely have a // unit-test for this case. Specifically, I'd suggest
// unit-test InstCombine
pub fn redundant_primitive_cast(x: *mut u8) -> *mut u8 {
x as *mut u8
}
pub fn redundant_cast_with_lifetimes<'a, 'b: 'a>(x: *const &'b u8) -> *const &'a u8 {
x as *mut u8
}I don't care care very much what this particular pass does
The goal of this kind of a test is to protect against someone introducing a bug into the pass that causes it not to do anything anymore (accidentally adds an if false, case gets forgotten in a refactor, that type of thing). You can additionally have a casts_e2e.rs test with all the PreCodegen.after examples you'd like.
There was a problem hiding this comment.
The InstCombine.diff for those examples are empty. I feel like there's something in MIR building that is skipping emitting the cast, and the situation I'm trying to optimize here only arises after inlining (which is why my test is the way it is)
saethlin
force-pushed
the
instcombine-redundant-casts
branch
from
13df8f3
to
f307f10
Compare
| @@ -142,6 +143,14 @@ impl<'tcx> InstCombineContext<'tcx, '_> { | |||
| } | |||
| } | |||
|
|
|||
| fn combine_cast(&self, _source_info: &SourceInfo, rvalue: &mut Rvalue<'tcx>) { | |||
| if let Rvalue::Cast(_kind, operand, ty) = rvalue { | |||
| if operand.ty(self.local_decls, self.tcx) == *ty { | |||
There was a problem hiding this comment.
Are regions erased in the MIR body here? Otherwise this won't apply any time a lifetime is present in the type being casted.
There was a problem hiding this comment.
Yes, they should be. Seems like a good candidate for a test case though
There was a problem hiding this comment.
Hm. I made an attempt, but I do not think it is optimal. Probably good enough though?
saethlin
force-pushed
the
instcombine-redundant-casts
branch
from
f307f10
to
0e05280
Compare
|
Since you're already looking at it :) |
|
Yeah, good enough for me. @bors r+ |
bors
added
S-waiting-on-bors
…llaumeGomez Rollup of 8 pull requests Successful merges: - rust-lang#108110 (Move some `InferCtxt` methods to `EvalCtxt` in new solver) - rust-lang#108168 (Fix ICE on type alias in recursion) - rust-lang#108230 (Convert a hard-warning about named static lifetimes into lint "unused_lifetimes") - rust-lang#108239 (Fix overlapping spans in removing extra arguments) - rust-lang#108246 (Add an InstCombine for redundant casts) - rust-lang#108264 (no-fail-fast support for tool testsuites) - rust-lang#108310 (rustdoc: Fix duplicated attributes for first reexport) - rust-lang#108318 (Remove unused FileDesc::get_cloexec) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Thanks to the combination of rust-lang#108246 and rust-lang#108442 it could already remove identity transmutes. With this PR, it can also simplify them to `IntToInt` casts, `Discriminant` reads, or `Field` projections.
…jgillot Simplify transmutes in MIR InstCombine Thanks to the combination of rust-lang#108246 and rust-lang#108442 it could already remove identity transmutes. With this PR, it can also simplify them to `IntToInt` casts, `Discriminant` reads, or `Field` projections.
@rustbot label +A-mir-opt