Simplify type_is_immediate and type_is_fat_ptr #38854
Conversation
| } | ||
| OperandValue::Pair(..) => bug!("Unexpected Pair operand") | ||
| }; | ||
| let discr = operand.immediate(); |
There was a problem hiding this comment.
You can lift let llval = operand.immediate() out of this if-let-else.
Mark-Simulacrum
force-pushed
the
immediate-refactor
branch
from
d80648f to
50dafd8
Compare
| _ => { | ||
| false | ||
| } | ||
| if let Layout::FatPointer { .. } = *ccx.layout_of(ty) { |
There was a problem hiding this comment.
Check that this does not regress rustc performance - this fn is called quite often.
There was a problem hiding this comment.
The layouts are cached and sometime during compilation the layout of any type will be requested anyway, so it seems fine to me?
There was a problem hiding this comment.
I can do some benchmarking with regards to this, but it does feel needless because they are cached.
|
Oh I didn't r+ this. What happened? |
| adt::trans_get_discr(&bcx, operand.ty, llptr, None, true) | ||
| } | ||
| OperandValue::Pair(..) => bug!("Unexpected Pair operand") | ||
| }; | ||
| let (signed, min, max) = match l { | ||
| &Layout::CEnum { signed, min, max, .. } => { |
There was a problem hiding this comment.
Can you do this if-let-else based on Layout and not CastTy?
Mark-Simulacrum
force-pushed
the
immediate-refactor
branch
from
50dafd8 to
aed90e1
Compare
|
Fixed. I can also pull in #38906, or let that be rebased on top of what I've done here once this merges. |
|
@Mark-Simulacrum I'd integrate #38906 (should be a small change to the size check). |
This has the nice benefit of making it much simpler to work with, since it now consists of a single match statement.
Mark-Simulacrum
force-pushed
the
immediate-refactor
branch
from
aed90e1 to
b6bf8a1
Compare
|
Tests pass locally for me. |
| Layout::UntaggedUnion { .. } | | ||
| Layout::RawNullablePointer { .. } | | ||
| Layout::StructWrappedNullablePointer { .. } => { | ||
| !layout.is_unsized() && type_is_zero_size(ccx, ty) |
There was a problem hiding this comment.
You should still be checking the layout size.
There was a problem hiding this comment.
I believe that I've exactly mirrored @dotdash's PR. The previous size comparison changes to a "is_zero_size" check for all types not matched above, which is what the code in this PR currently does. If not, please inform me what I am missing.
- match ty.sty {
- ty::TyAdt(..) | ty::TyTuple(..) | ty::TyArray(..) | ty::TyClosure(..) => {
- let llty = sizing_type_of(ccx, ty);
- llsize_of_alloc(ccx, llty) <= llsize_of_alloc(ccx, ccx.int_type())
- }
- _ => type_is_zero_size(ccx, ty)
- }
+ type_is_zero_size(ccx, ty)There was a problem hiding this comment.
What I'm saying is you have a layout and you can get the size directly, i.e. you change the threshold in your original patch.
There was a problem hiding this comment.
So instead of type_is_zero_size I would use layout.size() == 0 (or the equivalent if there's a method)?
"You change the threshold in your original patch" I don't know what you mean by this...
There was a problem hiding this comment.
Yes, that. If you use that it will look like what you had here before, but instead of <= dl.ptr_size.bytes() you'll have == 0.
LLVM usually prefers using memcpys over direct loads/store of first class aggregates. The check in type_is_immediate to mark certain small structs was originally part of the code to handle such immediates in function arguments, and it had a counterpart in load_ty/store_ty to actually convert small aggregates to integers. But since then, the ABI handling has been refactored and takes care of converting small aggregates to integers. During that refactoring, the code to handle small aggregates in load_ty/store_ty has been removed, and so we accidentally started using loads/stores on FCA values. Since type_is_immediate() is no longer responsible for ABI-related conversions, and using a memcpy even for small aggregates is usually better than performing a FCA load/store, we can remove that code part and only handle simple types as immediates there. This integrates PR rust-lang#38906 onto this branch. Fixes rust-lang#38906.
Mark-Simulacrum
force-pushed
the
immediate-refactor
branch
from
b6bf8a1 to
43cf5b9
Compare
|
Tests pass locally. I think I resolved @eddyb's last comment. |
|
@bors r+ |
|
📌 Commit 43cf5b9 has been approved by |
Simplify type_is_immediate and type_is_fat_ptr r? @eddyb
|
☀️ Test successful - status-appveyor, status-travis |
r? @eddyb