checked_div happy path

[leonardo-m]

This is a kind of enhancement suggestion. Given this function:

pub fn foo7(n: u32, v: u32) -> Option<u32> {
    n.checked_div(v + 1)
}

Compiled wih rustc 1.46.0-nightly (67100f6 2020-06-24) with good optimization flags gives:

example::foo7:
	inc     esi
	jne     .LBB0_3
	xor     eax, eax
	ret
.LBB0_3:
	mov     eax, edi
	xor     edx, edx
	div     esi
	mov     edx, eax
	mov     eax, 1
	ret

But I think the happy path should be with the denominator != 0 (that hopefully is the most common case), instead of the case with a division by zero.

So I've done few more experiments:

#![feature(core_intrinsics)]
use std::intrinsics::{unchecked_div, unlikely, likely};

pub fn checked_div1(this: u32, rhs: u32) -> Option<u32> {
    match rhs {
        0 => None,
        rhs => Some(unsafe { unchecked_div(this, rhs) }),
    }
}

pub fn foo1(n: u32, v: u32) -> Option<u32> {
    checked_div1(n, v + 1)
}

pub fn checked_div2(this: u32, rhs: u32) -> Option<u32> {
    if rhs != 0 {
        Some(unsafe { unchecked_div(this, rhs) })
    } else {
        None         
    }
}

pub fn foo2(n: u32, v: u32) -> Option<u32> {
    checked_div2(n, v + 1)
}

pub fn checked_div3(this: u32, rhs: u32) -> Option<u32> {
    if unlikely(rhs == 0) {
        None
    } else {
        Some(unsafe { unchecked_div(this, rhs) })
    }
}

pub fn foo3(n: u32, v: u32) -> Option<u32> {
    checked_div3(n, v + 1)
}

pub fn foo4(n: u32, v: u32) -> Option<u32> {
    if likely(v + 1 != 0) {
        Some(unsafe { unchecked_div(n, v + 1) })
    } else {
        None        
    }
}

The asm:

example::checked_div1:
	test    esi, esi
	je      .LBB0_1
	mov     eax, edi
	xor     edx, edx
	div     esi
	mov     edx, eax
	mov     eax, 1
	ret
.LBB0_1:
	xor     eax, eax
	ret

example::foo1:
	inc     esi
	jne     .LBB1_2
	xor     eax, eax
	ret
.LBB1_2:
	mov     eax, edi
	xor     edx, edx
	div     esi
	mov     edx, eax
	mov     eax, 1
	ret

This issue has been assigned to @nbdd0121 via this comment.

[nagisa]

Ideally/most cleanly solved with some branch weight annotation that we don’t have implemented yet.

I wouldn’t rely much on re-arranging Rust code much as that can break again on a whim.

[leonardo-m]

Isn't std::intrinsics::likely a branch weight annotation? Isn't it working in Rust?

[nagisa]

Oh I guess that ended up being added, I skimmed past that part in your report. Yeah in that case we should just go with one of the if unlikely options.

[nbdd0121]

I had a try, but it cannot yet be implemented because likely/unlikely are not yet const intrinsics.

[nbdd0121]

Okay I have a working version here. However this would be blocked until likely and unlikely is made const.

@rustbot claim

[nagisa]

@oli-obk @RalfJung any reason we cannot or should not make the likely/unlikely intrinsic const?

[RalfJung]

None that I can think of. But this requires T-lang approval (through FCP I presume) as it is an intrinsic. Better make it a separate PR.

[nbdd0121]

@rustbot modify labels: -S-blocked