Extra null check in slice iterator's `.next().is_none()`

[bluss]

This is an interesting codegen bug (Note that this reproduces in a special case setting and in many regular contexts, this problem does not exist).

Expected behavior:

This code should just be a pointer comparison (ptr == end leads to None being returned)

use std::slice::Iter;
pub fn is_empty_1(xs: Iter<f32>) -> bool {
    {xs}.next().is_none()
}

Actual behavior:

It compiles to the equivalent of ptr == end || ptr.is_null().

Playground link. Use release mode.

version: rustc 1.15.0-nightly (0bd2ce6 2016-11-19)

Additional notes:

This version has the expected codegen, without the null check.

pub fn is_empty_2(xs: Iter<f32>) -> bool {
    xs.map(|&x| x).next().is_none()
}

[arielb1]

We just plain never emit nonnull metadata, don't we? Ah, we do have an assume. Need to figure out why it is being eaten.

[bluss]

Assume shouldn't even be needed, should it. Creating a shared reference should be enough of a non-null assertion.

[arielb1]

It's SROA this time, eating !nonnull when it is on an alloca:

define zeroext i1 @sroa_fail(float**) {
entry-block:
  %buf0 = alloca float*
  %buf1 = alloca i8*

  %load0 = load float*, float** %0, align 8
  store float* %load0, float** %buf0

  %load1 = load float*, float** %buf0, align 8, !nonnull !0

  %bc = bitcast float* %load1 to i8*
  store i8* %bc, i8** %buf1, align 8

  %load2 = load i8*, i8** %buf1, align 8
  %ret = icmp eq i8* %load2, null
  ret i1 %ret
}

!0 = !{}

The first load/store pair comes from a lowered memcpy.

[arielb1]

LLVM bug https://llvm.org/bugs/show_bug.cgi?id=31142.

[luqmana]

I hacked together an LLVM patch that preserves the nonnull metadata on Loads in SROA/mem2reg: luqmana/llvm@da58c74

With that, the extraneous null check is gone:

define zeroext i1 @_ZN9sroa_fail10is_empty_117hfeb3b6a38ecff321E(%"core::slice::Iter<f32>"* noalias nocapture readonly dereferenceable(16)) unnamed_addr #0 {
entry-block:
  %xs.sroa.0.0..sroa_idx = getelementptr inbounds %"core::slice::Iter<f32>", %"core::slice::Iter<f32>"* %0, i64 0, i32 0
  %xs.sroa.0.0.copyload = load float*, float** %xs.sroa.0.0..sroa_idx, align 8, !nonnull !0
  %xs.sroa.4.0..sroa_idx12 = getelementptr inbounds %"core::slice::Iter<f32>", %"core::slice::Iter<f32>"* %0, i64 0, i32 1
  %xs.sroa.4.0.copyload = load float*, float** %xs.sroa.4.0..sroa_idx12, align 8, !nonnull !0
  %1 = icmp eq float* %xs.sroa.0.0.copyload, %xs.sroa.4.0.copyload
  ret i1 %1
}

[bluss]

that particular nonnull, where does it come from? The assume?

[bluss]

My question asked differently, will this also have an extra null check?

pub struct MockIter {
    start: *const f32,
    end: *const f32,
}

impl MockIter {
    unsafe fn next<'a>(&mut self) -> Option<&'a f32> {
        if self.start != self.end {
            let ptr = self.start;
            self.start = self.start.offset(1);
            Some(&*ptr)
        } else {
            None
        }
    }
}

pub fn is_empty_3(xs: MockIter) -> bool {
    unsafe {
        {xs}.next().is_none()
    }
}

I am keen on having stable rust (no assume superpowers) be the best it can be.

[luqmana]

@bluss The nonnull metadata do come from the assume; the InstCombine pass turns assume( (load addr) != null ) to a load addr !nonnull.
So unfortunately is_empty_3 would still have the extra null check.

[bluss]

Even a simple identity function fn non_null(x: &f32) -> &f32 has the attribute dereferenceable(4) on the return value. Maybe it can be used to insert a non null tag here?

[bluss]

Can I help with getting that patch upstreamed somehow?

[luqmana]

@bluss I've submitted it upstream ( https://reviews.llvm.org/D27114 )

[bluss]

Are all the three parts needed in the patch? Since one was apparently not a valid optimization.