Let rvalue_creates_operand return true for *all* Rvalue::Aggregates
#143502
Conversation
rustbot
added
the
T-compiler
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
Let `rvalue_creates_operand` return true for *all* `Rvalue::Aggregate`s Draft for now because it's built on Ralf's #143291 Inspired by #138759 (comment) where I noticed that we were nearly at this point, plus the comments I was writing in #143410 that reminded me a type-dependent `true` is fine. This PR splits the `OperandRef::builder` logic out to a separate type, with the updates needed to handle SIMD as well. In doing so, that makes the existing `Aggregate` path in `codegen_rvalue_operand` capable of handing SIMD values just fine. As a result, we no longer need to do layout calculations for aggregate result types when running the analysis to determine which things can be SSA in codegen.
This comment has been minimized.
This comment has been minimized.
| Immediate(Either<V, abi::Scalar>), | ||
| Pair(Either<V, abi::Scalar>, Either<V, abi::Scalar>), | ||
| /// SIMD vectors need special handling because they're the only case where | ||
| /// a type with a (non-ZST) `Memory`-ABI field can be `Scalar`-ABI. |
There was a problem hiding this comment.
I don't understand this comment... BackendRepr::Memory and BackendRepr::SimdVector are mutually exclusive, how can a type be both?
"ABI" is such an overused term, it may be good to use some more precise term here if there is one.
There was a problem hiding this comment.
I agree it's weird for a SimdVector to have Memory fields, but that's what repr(simd) does :P
Hopefully the new comment is clearer.
There was a problem hiding this comment.
Ah, I missed the fact that "field" is the key term here. :)
Yeah that is kind of odd... might be worth changing? Haven't you been working on entirely forbidding field projections for SIMD vectors?
There was a problem hiding this comment.
Yeah (rust-lang/compiler-team#838), but technically that's just about projections and there's no ProjectionElem involved here, just the Rvalue::Aggregate.
We've talked about potentially requiring transmutes to introduce them too, not just for consuming them, but it's not a thing yet.
There was a problem hiding this comment.
TBH if just letting it be supported only takes the couple of lines here (https://github.com/rust-lang/rust/pull/143502/files#diff-013fea668c72159276fa60d94808141397bb0ec0f53f93c79c91335cbcda135eR681-R688) then I probably wouldn't MCP banning it. The big reason that projections are annoying is things like borrowing them or setting sub-fields (like _2.0[3] = _4; when _2 is repr(simd)), but just the Aggregate doesn't have those problems.
There was a problem hiding this comment.
Oh this is just for constructors? I see.
I didn't dig into the code, will trust you on there not being obvious improvements here. :)
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (3b510de): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (secondary -1.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 3.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 461.008s -> 459.649s (-0.29%) |
rustbot
removed
the
S-waiting-on-perf
rustbot
added
the
S-waiting-on-review
|
Some changes occurred in compiler/rustc_codegen_ssa |
Inspired by <rust-lang#138759 (comment)> where I noticed that we were nearly at this point, plus the comments I was writing in 143410 that reminded me a type-dependent `true` is fine. This PR splits the `OperandRef::builder` logic out to a separate type, with the updates needed to handle SIMD as well. In doing so, that makes the existing `Aggregate` path in `codegen_rvalue_operand` capable of handing SIMD values just fine. As a result, we no longer need to do layout calculations for aggregate result types when running the analysis to determine which things can be SSA in codegen.
Don't require `alloca`s for consuming simple enums Well, 4 months later I'm finally back to this. Will stay as Draft until #143502 lands, since this builds on that. For example, if you pass an `Option<u32>` to a function, don't immediately write it to an `alloca` then read it again.
Draft for now because it's built on Ralf's #143291Inspired by #138759 (comment) where I noticed that we were nearly at this point, plus the comments I was writing in #143410 that reminded me a type-dependent
trueis fine.This PR splits the
OperandRef::builderlogic out to a separate type, with the updates needed to handle SIMD as well. In doing so, that makes the existingAggregatepath incodegen_rvalue_operandcapable of handing SIMD values just fine.As a result, we no longer need to do layout calculations for aggregate result types when running the analysis to determine which things can be SSA in codegen.