rustc: mark references w/anonymous lifetime nocapture

[emberian]

Closes #6751

[emberian]

r? @thestinger @nikomatsakis

[thestinger]

@cmr: Can you make sure they are marked nocapture both in the declaration itself and on calls of function pointers? I seem to remember this code being split in two places, but maybe it was fixed.

[emberian]

@thestinger what would IR for that look like? For example with

fn foo<'a>(x: &'a int) -> int { *x }
fn bar(x: &int) -> int { *x }

fn unwrap<T>(f: fn(&T) -> T, v: &T) -> T {
    f(v)
}

fn main() {
    let x = unwrap(foo, &42);
    let y = unwrap(bar, &32);
    assert!(x == 42);
    assert!(y == 32);
}

I find a line in the IR:

%9 = call i64 @_ZN6unwrap20hacdc2b0bbf46ca93Daa4v0.0E(i64 (i64*)* @_ZN3bar20h161c05615de3eaa4saa4v0.0E, i64* %addr_of1)

Which if I change to...

%9 = call i64 @_ZN6unwrap20hacdc2b0bbf46ca93Daa4v0.0E(i64 (i64* nocapture)* @_ZN3bar20h161c05615de3eaa4saa4v0.0E, i64* %addr_of1)

llc dies with

llc: foo.ll:341:62: error: argument attributes invalid in function type

[thestinger]

@cmr: I forget the syntax, but the code adding sret is in src/librustc/middle/trans/callee.rs.

        // A function pointer is called without the declaration
        // available, so we have to apply any attributes with ABI
        // implications directly to the call instruction. Right now,
        // the only attribute we need to worry about is `sret`.
        let mut attrs = Vec::new();
        if type_of::return_uses_outptr(ccx, ret_ty) {
            attrs.push((1, StructRetAttribute));
        }

        // The `noalias` attribute on the return value is useful to a
        // function ptr caller.
        match ty::get(ret_ty).sty {
            // `~` pointer return values never alias because ownership
            // is transferred
            ty::ty_uniq(..) | ty::ty_vec(_, ty::vstore_uniq) => {
                attrs.push((0, NoAliasAttribute));
            }
            _ => {}
        }

So you could add nocapture there and see if it works, or if LLVM just doesn't allow this.

[emberian]

@thestinger after playing with it, LLVM errors on broken functions whenever I try to add attributes there. So I think it just doesn't allow it.

[dotdash]

The code that @thestinger referenced is about adding the attribute to the argument in the call itself. For example, there is an indirect call in unwrap() that looks like this:

%4 = call i64 %2(i64* %3)

extending the code to handle nocapture as well should turn that into:

  %4 = call i64 %2(i64* nocapture %3)

[dotdash]

Made up example where this makes a difference:

; ModuleID = 'issue-13001.rs'
target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

declare void @other()

define i64 @without(i64 (i64*)*) unnamed_addr #4 {
entry-block:
  %1 = alloca i64
  %2 = call i64 %0(i64* %1)
  store i64 8, i64* %1
  call void @other()
  ret i64 %2
}

define i64 @with(i64 (i64*)*) unnamed_addr #4 {
entry-block:
  %1 = alloca i64
  %2 = call i64 %0(i64* nocapture %1)
  store i64 8, i64* %1
  call void @other()
  ret i64 %2
}

Gets optimized to:

; ModuleID = '<stdin>'
target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

declare void @other()

define i64 @without(i64 (i64*)* nocapture) unnamed_addr {
entry-block:
  %1 = alloca i64, align 8
  %2 = call i64 %0(i64* %1)
  store i64 8, i64* %1, align 8
  call void @other()
  ret i64 %2
}

define i64 @with(i64 (i64*)* nocapture) unnamed_addr {
entry-block:
  %1 = alloca i64, align 8
  %2 = call i64 %0(i64* nocapture %1)
  tail call void @other()
  ret i64 %2
}

The store can only be removed if nocapture is present.

[thestinger]

@cmr: It's already adding sret there so it does seem to work.

[emberian]

What I tried in the "wip" commit.

[emberian]

(Rebuilding to try and debug further)

[emberian]

Current status with the wips:

Attribute after last parameter!
  call void %34(%"enum.result::Result<(),io::IoError>"* noalias nocapture sret %0, i8* %28, { i8*, i64 }* nocapture %__adjust)
Attribute after last parameter!
  call void %67(%"enum.result::Result<(),io::IoError>"* noalias nocapture sret %0, i8* %61, { i8*, i64 }* nocapture %__adjust2)
Attribute after last parameter!
  call void %88(%"enum.result::Result<(),io::IoError>"* noalias nocapture sret %0, i8* %82, { i8*, i64 }* nocapture %__adjust13)
Attribute after last parameter!
  call void %104(%"enum.result::Result<(),io::IoError>"* noalias nocapture sret %0, i8* %98, { i8*, i64 }* nocapture %__adjust14)
LLVM ERROR: Broken function found, compilation aborted!

I think the accounting for self/envptr might be wrong but I'll continue debugging tomorrow.

[dotdash]

Note that you don't have to account for self, that should be a regular argument as far as ty_fn_args is concerned. You probably missed the gist I pasted in #rust-internals earlier today, https://gist.github.com/dotdash/163790858bc88d5c66ac -- that worked fine with ZExt for me.

[emberian]

@dotdash ah thanks. do you want me to add that to this pr?

[nikomatsakis]

@cmr looks like travis doesn't like some of your long lines. I gave r+ but removed it due to travis failures.

I think the reasoning for using NoCapture here is valid, and the code looks right to me, but I confess I'm having some trepidation. I'm starting to think I'd rather see this sort of analysis take place in a pass before trans. The workings of trans are so confusing that I feel like I don't have a good grasp on the complete set of conditions in which, e.g., register_fn can be called. I'm also worried about how complicated and confusing it's going to be as the set of conditions we look for grows.

I think I'd feel more comfortable if we had a pass that ran just before trans and which analysed and annotated (in a side table) the various parameters with the various kinds of attributes that they are best suited for. I envision a very simple visitor that walks the AST, examining the declared types of each function and closure.

One argument against this approach would be that perhaps the results of monomorphization are relevant in some cases (though not in this one). There would also be the necessity of serializing and deserializing these results for cross-crate inlining, which is always a bit of a pain.

@cmr what do you think about that? Am I just being overly nervous?

[thestinger]

@nikomatsakis: At some point we'll want to be outputting TBAA metadata nodes rather than just parameter attributes, so we will need some kind of smarter high-level infrastructure. I don't think it's too difficult to get it right with just parameters though.

[emberian]

These failures are in some way legitimate, investigating.

[emberian]

Accidentally let LLVM do some TCO where we didn't want it.