Employ non-null metadata for loads from fat pointers #27180
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
r? @pnkfelix Would it be possible to add a codegen test for an optimization that this enables? |
|
@alexcrichton the problem here is that, AFAIK, we cannot mark tests to be run only when optimizations are enabled, and using rustflags:-O would pass -O twice which makes rustc complain. Does anyone know if there's a way around that limitation? |
|
Could the compiletest driver filter out all |
|
We need some way to tag the IR tests as only-opt or only-arch-x etc. |
|
I guess @luqmana is right in that tagging tests to only run for certain options/targets is what we need as it is strictly more powerful and that would also allow for things like ABI-compliance tests. I don't feel like touching the compiletest code just now though... Maybe next week, unless somebody wants to beat me to it. |
| ty::TyBox(_) | ty::TyRef(..) => { | ||
| LoadNonNull(bcx, addr_ptr) | ||
| } | ||
| _ => Load(bcx, addr_ptr) |
There was a problem hiding this comment.
What's the point of checking the type here? A fat pointer is by definition pointing at something.
There was a problem hiding this comment.
I'm excluding TyRawPtr here because I expected those to be allowed to have a data pointer that points at null.
There was a problem hiding this comment.
Mmm... that's right, I think. I was getting confused a bit because this is used to load both fat pointers and dereferences of fat pointers.
|
You aren't passing the correct type to load_addr and load_extra consistently; some places pass a pointer type, some places pass the pointee type. |
The data pointer inside a safe fat pointer is never null. The same is true for the vtable pointer inside a fat pointer for a trait object. Wrapping loads/stores of the components of a fat pointer in functions allows to easily make sure that we emit the appropriate metadata and allow LLVM to perform better optimizations.
We currently copy fat pointers using memcpy intrinsics, which SROA will split up into load/store pairs. Unfortunately, nonnull metadata on loads from said fat pointers might get lost in the process. So instead of using memcpy we can go ahead and use load/store pairs right away including the appropriate nonnull metadata on the loads. That way the metadata doesn't get lost and LLVM has better chances of eliminating null checks. One example for this is the code in rust-lang#27130 that currently has an extra null check that disappears after this change.
|
Since the data behind fat pointers is opaque to rust code, these functions are only used in contexts where the fat pointer is effectively being dereferenced anyway, so I made Thanks for the feedback @eefriedman! |
| @@ -960,7 +979,9 @@ pub fn memcpy_ty<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, | |||
| t: Ty<'tcx>) { | |||
| let _icx = push_ctxt("memcpy_ty"); | |||
| let ccx = bcx.ccx(); | |||
| if t.is_structural() { | |||
| if common::type_is_fat_ptr(bcx.tcx(), t) { | |||
There was a problem hiding this comment.
Be sure you don't miscompile the likes of
pub fn foo() -> *const [u8] { unsafe { std::mem::transmute([0u64,0]) } }
There was a problem hiding this comment.
@arielb1 out of curiosity, what was the red flag here that made you worry about that test case? At first blush, I would have thought that expr::copy_fat_ptr will handle this correctly.
Update: ah, I see, there is a separate note on the commit where @areilb1 states some concern about the change to copy_fat_ptr itself, (and the example here is meant to provide a case where the data is not dereferenceable).
|
☔ The latest upstream changes (presumably #26173) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ah, this fell through the cracks on my end as well. @arielb1's concerns are valid and I need to address that. Will probably end up only adding the nonnull metadata at specific places instead. |
|
Closing due to inactivity, but feel free to resubmit with a rebase! |
These changes allow LLVM to perform better optimizations around fat pointers, e.g. eliminating the extraneous null check in #27130.