Skip to content

Improve handling of immediate values - #27600

Closed
Aatch wants to merge 2 commits into
rust-lang:masterfrom
Aatch:immediate-rvalue
Closed

Improve handling of immediate values#27600
Aatch wants to merge 2 commits into
rust-lang:masterfrom
Aatch:immediate-rvalue

Conversation

@Aatch

@AatchAatch commented Aug 8, 2015

Copy link
Copy Markdown
Contributor

The primary improvement here is to handle function calls that return
imemdiate values such that it doesn't force the use of a stack slot. It
also avoids loads from slots that have an store to it in the same block.

This especially improves the codegen for intrinsics, as most of them
produce immediate values.

cc @rust-lang/compiler

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @jroesch

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

Comment threadsrc/librustc_trans/trans/base.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd prefer to add that check after the if-else block, so it can be merged with the same one in the else-arm.

@dotdash

Copy link
Copy Markdown
Contributor

👍 I wanted to get rid of the store/load combo for function calls for sooooo long.

I'm not sure about the load elision thing. How often / in which situations does that come into play?

@brson

brson commented Aug 8, 2015

Copy link
Copy Markdown
Contributor

Neat. Do compile times improve?

The primary improvement here is to handle function calls that return
imemdiate values such that it doesn't force the use of a stack slot. It
also avoids loads from slots that have an store to it in the same block.
This especially improves the codegen for intrinsics, as most of them
produce immediate values.
* Move the "get the value that will definitely be loaded" logic to it's
own method to reduce the duplication.
* Refactor base::invoke to avoid duplicating the diverging/zero-sized
return logic.
* Re-add debugging output I accidentally removed
* Add some more comments explaining stuff
@Aatch

Copy link
Copy Markdown
ContributorAuthor

Updated to address comments.

@brson I haven't tested, but my previous attempt at this suggested, no. At least, nothing that isn't covered by noise. The main reason presumably being that let a = foo() and similar expressions still compile to about the same code, which probably accounts for a fair amount of function calls.

@jroesch

Copy link
Copy Markdown
Contributor

@Aatch read the patch over and it looks good to me, not sure if you want someone who spends more time in trans to look it over instead, otherwise r=me.

@dotdash

Copy link
Copy Markdown
Contributor

@bors r+ 8905343

@Manishearth

Copy link
Copy Markdown
Member

@bors

bors commented Aug 12, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 8905343 with merge 924bd8b...

@bors

bors commented Aug 12, 2015

Copy link
Copy Markdown
Collaborator

💔 Test failed - auto-mac-64-nopt-t

@bors

bors commented Aug 18, 2015

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #27169) made this pull request unmergeable. Please resolve the merge conflicts.

@jroesch

Copy link
Copy Markdown
Contributor

@Aatch ping, whats the status on this?

@dotdash

Copy link
Copy Markdown
Contributor

@Aatch Are you ok with me finishing this in a new PR?

@alexcrichton

Copy link
Copy Markdown
Member

Closing due to inactivity, but feel free to resubmit with a rebase! (also looks like @jroesch or @dotdash may be willing to help out on this)

Sign up for freeto 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.

8 participants

@Aatch@rust-highfive@dotdash@brson@jroesch@Manishearth@bors@alexcrichton