Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

librustc: Don't create extra alloca slot for by value bindings in match. #15076

Merged
merged 4 commits into from
Jul 3, 2014

Conversation

luqmana
Copy link
Member

@luqmana luqmana commented Jun 21, 2014

struct With {
    x: int,
    f: NoCopy
}

#[no_mangle]
fn bar() {
    let mine = With { x: 3, f: NoCopy };
    match mine {
        c => {
            foo(c);
        }
    }
}

#[no_mangle]
fn foo(_: With) {}

Before:

define internal void @bar() unnamed_addr #1 {
entry-block:
  %mine = alloca %"struct.With<[]>"
  %__llmatch = alloca %"struct.With<[]>"*
  %c = alloca %"struct.With<[]>"
  %0 = getelementptr inbounds %"struct.With<[]>"* %mine, i32 0, i32 0
  store i64 3, i64* %0
  %1 = getelementptr inbounds %"struct.With<[]>"* %mine, i32 0, i32 1
  store %"struct.With<[]>"* %mine, %"struct.With<[]>"** %__llmatch
  br label %case_body

case_body:                                        ; preds = %entry-block
  %2 = load %"struct.With<[]>"** %__llmatch
  %3 = bitcast %"struct.With<[]>"* %2 to i8*
  %4 = bitcast %"struct.With<[]>"* %c to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %4, i8* %3, i64 8, i32 8, i1 false)
  %5 = load %"struct.With<[]>"* %c
  call void @foo(%"struct.With<[]>" %5)
  br label %join

join:                                             ; preds = %case_body
  ret void
}

After:

define internal void @bar() unnamed_addr #1 {
entry-block:
  %mine = alloca %"struct.With<[]>"
  %c = alloca %"struct.With<[]>"*
  %0 = getelementptr inbounds %"struct.With<[]>"* %mine, i32 0, i32 0
  store i64 3, i64* %0
  %1 = getelementptr inbounds %"struct.With<[]>"* %mine, i32 0, i32 1
  store %"struct.With<[]>"* %mine, %"struct.With<[]>"** %c
  br label %case_body

case_body:                                        ; preds = %entry-block
  %2 = load %"struct.With<[]>"** %c
  %3 = load %"struct.With<[]>"* %2
  call void @foo(%"struct.With<[]>" %3)
  br label %join

join:                                             ; preds = %case_body
  ret void
}

r? @pcwalton

@pcwalton
Copy link
Contributor

Does this correctly handle:

let mut x = 3;
match x {
    mut y => { y = 5; }
}

In that case you must actually create an extra slot.

@pcwalton
Copy link
Contributor

(Actually I don't think you need to create an extra slot per binding, but you should create one for the matched value.)

@pcwalton
Copy link
Contributor

@luqmana I think what you need to do is to make a temporary copy of the value in the head of the match if and only if the matched value is Copy AND (there are mutable by-value bindings OR the value is not Share). That's because Cells can mess things up too. For a first cut it may be simpler to just unconditionally make a temporary copy if the value is Copy.

This is great work BTW, thanks!

@pcwalton
Copy link
Contributor

I think this may be wrong in some cases: what about

match box Foo { x: String::new("hi") } {
    box Foo { x } => ... use x ...
}

This might result in use-after-free. I think we may have to restrict the optimization to cases in which it's a local variable or an rvalue guaranteed to be on the stack that you're moving out of.

@pcwalton
Copy link
Contributor

@bors: retry

bors added a commit that referenced this pull request Jul 3, 2014
```Rust
struct With {
    x: int,
    f: NoCopy
}

#[no_mangle]
fn bar() {
    let mine = With { x: 3, f: NoCopy };
    match mine {
        c => {
            foo(c);
        }
    }
}

#[no_mangle]
fn foo(_: With) {}
```

Before:
```LLVM
define internal void @bar() unnamed_addr #1 {
entry-block:
  %mine = alloca %"struct.With<[]>"
  %__llmatch = alloca %"struct.With<[]>"*
  %c = alloca %"struct.With<[]>"
  %0 = getelementptr inbounds %"struct.With<[]>"* %mine, i32 0, i32 0
  store i64 3, i64* %0
  %1 = getelementptr inbounds %"struct.With<[]>"* %mine, i32 0, i32 1
  store %"struct.With<[]>"* %mine, %"struct.With<[]>"** %__llmatch
  br label %case_body

case_body:                                        ; preds = %entry-block
  %2 = load %"struct.With<[]>"** %__llmatch
  %3 = bitcast %"struct.With<[]>"* %2 to i8*
  %4 = bitcast %"struct.With<[]>"* %c to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %4, i8* %3, i64 8, i32 8, i1 false)
  %5 = load %"struct.With<[]>"* %c
  call void @foo(%"struct.With<[]>" %5)
  br label %join

join:                                             ; preds = %case_body
  ret void
}
```

After:
```LLVM
define internal void @bar() unnamed_addr #1 {
entry-block:
  %mine = alloca %"struct.With<[]>"
  %c = alloca %"struct.With<[]>"*
  %0 = getelementptr inbounds %"struct.With<[]>"* %mine, i32 0, i32 0
  store i64 3, i64* %0
  %1 = getelementptr inbounds %"struct.With<[]>"* %mine, i32 0, i32 1
  store %"struct.With<[]>"* %mine, %"struct.With<[]>"** %c
  br label %case_body

case_body:                                        ; preds = %entry-block
  %2 = load %"struct.With<[]>"** %c
  %3 = load %"struct.With<[]>"* %2
  call void @foo(%"struct.With<[]>" %3)
  br label %join

join:                                             ; preds = %case_body
  ret void
}
```

r? @pcwalton
@bors bors closed this Jul 3, 2014
@bors bors merged commit 77f72d3 into rust-lang:master Jul 3, 2014
@luqmana luqmana deleted the naim branch July 5, 2014 02:46
bors added a commit that referenced this pull request Jul 5, 2014
Inadvertently changed the order in which destructors ran in certain cases with #15076.

Fixes #15438.
@huonw
Copy link
Member

huonw commented Jul 5, 2014

This apparently caused a 1 GB memory regression!

@huonw
Copy link
Member

huonw commented Jul 5, 2014

(IRC informs me that #15442 is the fix; nice catch @dotdash, nice fix @luqmana.)

@lilyball
Copy link
Contributor

lilyball commented Aug 1, 2014

I think this is the cause for the double-drop seen in #16151.

lnicola pushed a commit to lnicola/rust that referenced this pull request Jun 19, 2023
internal: Shrink size of hir::Binding
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants