Skip to content

Simplify the codegen in iter_vec_loop #22438

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

Merged
merged 1 commit into from
Feb 19, 2015
Merged

Conversation

dotdash
Copy link
Contributor

@dotdash dotdash commented Feb 17, 2015

No need to create a bunch of blocks and a stack allocated temporary just
to build a simple loop.

No need to create a bunch of blocks and a stack allocated temporary just
to build a simple loop.
@rust-highfive
Copy link
Contributor

r? @pnkfelix

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

let i = Load(inc_bcx, loop_counter);
let plusone = Add(inc_bcx, i, C_uint(bcx.ccx(), 1us), DebugLoc::None);
Store(inc_bcx, plusone, loop_counter);
let lleltptr = if vt.llunit_alloc_size == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we could just bracket the whole loop in this and avoid emitting anything at all for zero sized types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The f callback might emit code that has side-effects WRT something else than the array itself, we can't just omit that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, of course.

@huonw
Copy link
Member

huonw commented Feb 17, 2015

I've been lead to believe that emitting phi nodes directly is generally frowned upon, I guess its fine for such a localised internals change (i.e. it's not a loop the user writes directly)?

@dotdash
Copy link
Contributor Author

dotdash commented Feb 17, 2015

I've been lead to believe that emitting phi nodes directly is generally frowned upon, I guess its fine for such a localised internals change (i.e. it's not a loop the user writes directly)?

Never heard of that, but now that you mentioned it, I decided to check what clang does, and it emits almost the same code that rustc produces with this patch. Phis are used there when building the loops for array initialization and destruction, same thing this code is used for. Only difference is that clang uses the pointer as the loop var instead of a counter, but the resulting optimized code seems identical and using the counter allows for the same code to be generated for zero-sized element types.

C++ code:

struct X {
  X() {}
};

void foo() {
  X x[1000];
}

Clang emitted IR:

define void @_Z3foov() #0 {
  %x = alloca [1000 x %struct.X], align 16
  %1 = getelementptr inbounds [1000 x %struct.X]* %x, i32 0, i32 0
  %2 = getelementptr inbounds %struct.X* %1, i64 1000
  br label %3

; <label>:3                                       ; preds = %3, %0
  %4 = phi %struct.X* [ %1, %0 ], [ %5, %3 ]
  call void @_ZN1XC2Ev(%struct.X* %4)
  %5 = getelementptr inbounds %struct.X* %4, i64 1
  %6 = icmp eq %struct.X* %5, %2
  br i1 %6, label %7, label %3

; <label>:7                                       ; preds = %3
  ret void
}

Rust code:

fn main() {
    let x = [0; 1000];
}

rustc emitted IR (with the patch):

define internal void @_ZN4main20hed4aaa978de9fce6eaaE() unnamed_addr #0 {
entry-block:
  %x = alloca [1000 x i32]
  %0 = getelementptr inbounds [1000 x i32]* %x, i32 0, i32 0
  br label %expr_repeat

expr_repeat:                                      ; preds = %expr_repeat, %entry-block
  %1 = phi i64 [ 0, %entry-block ], [ %3, %expr_repeat ]
  %2 = getelementptr inbounds i32* %0, i64 %1
  store i32 0, i32* %2
  %3 = add i64 %1, 1
  %4 = icmp ult i64 %3, 1000
  br i1 %4, label %expr_repeat, label %"expr_repeat: next"

"expr_repeat: next":                              ; preds = %expr_repeat
  ret void
}

Had to use an object instead of an integer in C++ because that's the only way to get array initialization that does a loop.

@huonw
Copy link
Member

huonw commented Feb 18, 2015

@bors r+ 378a

@huonw huonw assigned huonw and unassigned pnkfelix Feb 18, 2015
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Feb 19, 2015
No need to create a bunch of blocks and a stack allocated temporary just
to build a simple loop.
@bors bors merged commit 378abdb into rust-lang:master Feb 19, 2015
@dotdash dotdash deleted the iter_vec_loop branch March 1, 2015 19:21
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