Introduce unsafe offset_from on pointers #49297

scottmcm · 2018-03-23T08:35:31Z

Adds intrinsics::exact_div to take advantage of the unsafe, which reduces the implementation from

    sub rcx, rdx
    mov rax, rcx
    sar rax, 63
    shr rax, 62
    lea rax, [rax + rcx]
    sar rax, 2
    ret

down to

    sub rcx, rdx
    sar rcx, 2
    mov rax, rcx
    ret

(for *const i32)

See discussion on the offset_to tracking issue #41079

Some open questions

Would you rather I split the intrinsic PR from the library PR?
Do we even want the safe version of the API? Tracking issue for ptr::offset_from (feature: ptr_offset_from) #41079 (comment) I've added some text to its documentation that even if it's not UB, it's useless to use it between pointers into different objects.

and todos

~~I need to make a codegen test~~ Done
~~Can the subtraction use nsw/nuw?~~ No, it can't Introduce unsafe offset_from on pointers #49297 (comment)
Should there be usize variants of this, like there are now add and sub that you almost always want over offset? For example, I imagine sub_ptr that returns usize and where it's UB if the distance is negative. Can wait for later; C gives a signed result Tracking issue for ptr::offset_from (feature: ptr_offset_from) #41079 (comment), so we might as well, and this existing to go with offset makes sense.

Adds intrinsics::exact_div to take advantage of the unsafe, which reduces the implementation from ```asm sub rcx, rdx mov rax, rcx sar rax, 63 shr rax, 62 lea rax, [rax + rcx] sar rax, 2 ret ``` down to ```asm sub rcx, rdx sar rcx, 2 mov rax, rcx ret ``` (for `*const i32`)

rust-highfive · 2018-03-23T08:35:41Z

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

Amanieu · 2018-03-23T10:45:16Z

src/libcore/ptr.rs

+        assert!(0 < pointee_size && pointee_size <= isize::max_value() as usize);
+
+        // FIXME: can this be nuw/nsw?
+        let d = isize::wrapping_sub(self as _, other as _);


It can't be nuw because the result might be a negative value.

It can't be nsw either because an allocation may straddle the boundary between ISIZE_MAX (0x7fffffff) and ISIZE_MIN (0x80000000).

I agree. nuw and nsw force both the result and the operands to have the same interpretation as each other. What you'd really want here is a way to say that the operands have an unsigned interpretation, while the result has a signed interpretation, however there's currently no way to express that in LLVM.

Amanieu · 2018-03-23T10:46:43Z

src/librustc_trans/intrinsic.rs

@@ -343,6 +343,12 @@ pub fn trans_intrinsic_call<'a, 'tcx>(bx: &Builder<'a, 'tcx>,
                        "overflowing_add" => bx.add(args[0].immediate(), args[1].immediate()),
                        "overflowing_sub" => bx.sub(args[0].immediate(), args[1].immediate()),
                        "overflowing_mul" => bx.mul(args[0].immediate(), args[1].immediate()),
+                        "exact_div" =>
+                            if signed {
+                                bx.exactsdiv(args[0].immediate(), args[1].immediate())


exactsdiv seems to be missing?

It turned out to already exist: https://github.com/rust-lang/rust/pull/49297/files#diff-89b470c8d50892235012a9fffdb50d7cR361

strega-nil · 2018-03-23T12:29:42Z

src/libcore/ptr.rs

+    ///
+    /// let a = [0; 5];
+    /// let ptr1: *const i32 = &a[1];
+    /// let ptr2: *const i32 = &a[3];


This will panic at runtime. You do this a few other places as well. You should probably use get_unchecked? There's no actual mention of UB if you index out of bounds, so, I don't think it's an issue.

Huh? a is an array of length 5, so a[3] is not out of bounds.

(But if it was, get_unchecked would be UB because it uses offset, not wrapping_offset.)

I, uh, may be too used to OCaml, sorry -.-

Also, offset works with pointers which are one past the end.

~~Oh yeah if a had length 2 then that would have been fine.~~ Edit: Uh, yeah, see below

Actually, I'm just being stupid, array indexing is zero-based lol. Sorry, I should not comment when I'm tired :P

It was added 32 days after LLVM 3.9 shipped.

dtolnay

Looks good to me. Thanks!

I agree with the discussion in the tracking issue that the safe wrapping one seems suspicious, but I think it is fine to see how it gets used and revisit before stabilization.

dtolnay · 2018-03-25T11:39:58Z

@bors r+

bors · 2018-03-25T11:39:59Z

📌 Commit 6264952 has been approved by dtolnay

bors · 2018-03-26T00:15:40Z

⌛ Testing commit 6264952 with merge 39ee3aa...

Introduce unsafe offset_from on pointers Adds intrinsics::exact_div to take advantage of the unsafe, which reduces the implementation from ```asm sub rcx, rdx mov rax, rcx sar rax, 63 shr rax, 62 lea rax, [rax + rcx] sar rax, 2 ret ``` down to ```asm sub rcx, rdx sar rcx, 2 mov rax, rcx ret ``` (for `*const i32`) See discussion on the `offset_to` tracking issue #41079 Some open questions - Would you rather I split the intrinsic PR from the library PR? - Do we even want the safe version of the API? #41079 (comment) I've added some text to its documentation that even if it's not UB, it's useless to use it between pointers into different objects. and todos - [x] ~~I need to make a codegen test~~ Done - [x] ~~Can the subtraction use nsw/nuw?~~ No, it can't #49297 (comment) - [x] ~~Should there be `usize` variants of this, like there are now `add` and `sub` that you almost always want over `offset`? For example, I imagine `sub_ptr` that returns `usize` and where it's UB if the distance is negative.~~ Can wait for later; C gives a signed result #41079 (comment), so we might as well, and this existing to go with `offset` makes sense.

bors · 2018-03-26T02:39:22Z

☀️ Test successful - status-appveyor, status-travis
Approved by: dtolnay
Pushing 39ee3aa to master...

scottmcm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 23, 2018

rust-highfive assigned dtolnay Mar 23, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 23, 2018

scottmcm mentioned this pull request Mar 23, 2018

Tracking issue for ptr::offset_from (feature: ptr_offset_from) #41079

Closed

5 tasks

Amanieu reviewed Mar 23, 2018

View reviewed changes

strega-nil reviewed Mar 23, 2018

View reviewed changes

Add a codegen test for exact_div intrinsic

d6926ca

scottmcm force-pushed the offset-from branch from 39df78d to d6926ca Compare March 25, 2018 01:45

scottmcm added 2 commits March 24, 2018 19:15

Polyfill LLVMBuildExactUDiv

02b5851

It was added 32 days after LLVM 3.9 shipped.

Documentation and naming improvements

4a097ea

scottmcm changed the title ~~[WIP] Introduce unsafe offset_from on pointers~~ Introduce unsafe offset_from on pointers Mar 25, 2018

Fix doctest mutability copy-pasta

6264952

dtolnay approved these changes Mar 25, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 25, 2018

bors merged commit 6264952 into rust-lang:master Mar 26, 2018

scottmcm deleted the offset-from branch March 26, 2018 02:40

bors mentioned this pull request Mar 26, 2018

implement minmax intrinsics #49249

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce unsafe offset_from on pointers #49297

Introduce unsafe offset_from on pointers #49297

scottmcm commented Mar 23, 2018 •

edited

Loading

rust-highfive commented Mar 23, 2018

Amanieu Mar 23, 2018

sunfishcode Mar 23, 2018

Amanieu Mar 23, 2018

scottmcm Mar 24, 2018

strega-nil Mar 23, 2018 •

edited

Loading

hanna-kruppe Mar 23, 2018 •

edited

Loading

strega-nil Mar 23, 2018

strega-nil Mar 23, 2018

hanna-kruppe Mar 23, 2018 •

edited

Loading

strega-nil Mar 23, 2018

dtolnay left a comment

dtolnay commented Mar 25, 2018

bors commented Mar 25, 2018

bors commented Mar 26, 2018

bors commented Mar 26, 2018

Introduce unsafe offset_from on pointers #49297

Introduce unsafe offset_from on pointers #49297

Conversation

scottmcm commented Mar 23, 2018 • edited Loading

rust-highfive commented Mar 23, 2018

Amanieu Mar 23, 2018

Choose a reason for hiding this comment

sunfishcode Mar 23, 2018

Choose a reason for hiding this comment

Amanieu Mar 23, 2018

Choose a reason for hiding this comment

scottmcm Mar 24, 2018

Choose a reason for hiding this comment

strega-nil Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

hanna-kruppe Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

strega-nil Mar 23, 2018

Choose a reason for hiding this comment

strega-nil Mar 23, 2018

Choose a reason for hiding this comment

hanna-kruppe Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

strega-nil Mar 23, 2018

Choose a reason for hiding this comment

dtolnay left a comment

Choose a reason for hiding this comment

dtolnay commented Mar 25, 2018

bors commented Mar 25, 2018

bors commented Mar 26, 2018

bors commented Mar 26, 2018

scottmcm commented Mar 23, 2018 •

edited

Loading

strega-nil Mar 23, 2018 •

edited

Loading

hanna-kruppe Mar 23, 2018 •

edited

Loading

hanna-kruppe Mar 23, 2018 •

edited

Loading