Work around bad ptr::with_addr codegen in core::ptr#96538
Conversation
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
|
(rust-highfive has picked a reviewer for you, use r? to override) |
|
I got rid of it for now. It seems unfortunate, but I don't think we should penalize real current targets for this at the moment. That is, IMO code shouldn't have to pay a performance cost to switch to be compatible with strict provenance, even if the cost is fairly small. |
This comment has been minimized.
This comment has been minimized.
|
Hm, those are some downright spooky failures. |
310fc75 to
35f28d8
Compare
This comment has been minimized.
This comment has been minimized.
7253736 to
9a902b1
Compare
|
Okay, the fact that this fails for |
|
I don't know what's going on here. I did the self.cast::<u8>().wrapping_sub(self_addr).wrapping_add(addr).cast::<T>()@thomcc is speculating that this formulation looks to LLVM like |
|
Yeah, it's also possible that there's some bug I'm just not seeing in 35f28d8. |
|
It does get miscompiled for constant pointers: https://llvm.godbolt.org/z/chr3v7r9z I thought we had fixed this one already... |
|
Strictly speaking, the actual bug here is in the last step, but that's a well known issue. From a pragmatic point of view, we'd probably want to prevent the first fold, as it's generally not a good idea to convert a getelementptr into inttoptr and ptrtoint. |
|
@nikic what's your advice for this issue? Should we keep it as-is and close the PR? Does the code as-is (with the switched add/sub ordering) avoid this LLVM bug, or would it still be subject to it. |
|
@thomcc The switched order mostly avoids this, but I'm afraid the issue can still be hit: https://llvm.godbolt.org/z/eG4PMcbhb In this case the two GEPs will be interchanged to move one of them loop invariant, at which point we'd back to the same issue. |
|
Hm, I see. That's unfortunate. Probably enough to not want this change. |
|
I've put up https://reviews.llvm.org/D124677 to fix the miscompile. |
Fixes #96152.
Probably not really the ideal way to fix this, but the ideal way is an LLVM patch (or to make it an intrinsic), and I'm not a compiler person.