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

Don't create adjustments from a type to itself #28428

Merged
merged 1 commit into from
Sep 17, 2015

Conversation

dotdash
Copy link
Contributor

@dotdash dotdash commented Sep 15, 2015

Currently, we're generating adjustments, for example, to get from &[u8]
to &[u8], which is unneeded and kicks us out of trans_into()
into trans() which means an additional stack slot and copy in the
unoptimized code.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@dotdash
Copy link
Contributor Author

dotdash commented Sep 15, 2015

I just noticed that this fails in a number of cases, probably because of region mismatches in various &[u8] types. Is there a way to compare types while ignoring regions?

@nikomatsakis
Copy link
Contributor

The problem is that the adjustments are needed for the type system to be happy. I think we can trivially detect and optimize this sort of thing once MIR lands, however, which is probably my preferred strategy. Alternatively, it'd probably be fairly easy to detect and do a kind of peephole optimization for this case in trans.

@dotdash
Copy link
Contributor Author

dotdash commented Sep 16, 2015

Oh, right, I completely forgot about that. I guess the equality check as is
in this PR could be added though, right? That at least gets rid of a few
unnecessary copies.
Am 16.09.2015 18:42 schrieb "Niko Matsakis" notifications@github.com:

The problem is that the adjustments are needed for the type system to be
happy. I think we can trivially detect and optimize this sort of thing once
MIR lands, however, which is probably my preferred strategy. Alternatively,
it'd probably be fairly easy to detect and do a kind of peephole
optimization for this case in trans.


Reply to this email directly or view it on GitHub
#28428 (comment).

@nikomatsakis
Copy link
Contributor

@dotdash yeah, the equality check seems harmless. This by the way is an example of a case where it'd be nice to get some kind of unit test to help us ensure that micro-optimizations of generated code don't get lost.

@dotdash
Copy link
Contributor Author

dotdash commented Sep 16, 2015

Added a test. Some time soon, I'll remember to add those right away...

@nikomatsakis
Copy link
Contributor

@bors r+

@dotdash nice.

@bors
Copy link
Contributor

bors commented Sep 17, 2015

📌 Commit bc201fe has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Sep 17, 2015

⌛ Testing commit bc201fe with merge d8d8830...

@bors
Copy link
Contributor

bors commented Sep 17, 2015

💔 Test failed - auto-mac-32-opt

Currently, we're generating adjustments, for example, to get from &[u8]
to &[u8], which is unneeded and kicks us out of trans_into() into
trans() which means an additional stack slot and copy in the unoptimized
code.
@dotdash
Copy link
Contributor Author

dotdash commented Sep 17, 2015

@bors r=nikomatsakis 6def06c

sigh I skipped the size in the memcpy() call because I knew it's target dependent, but forgot that the same is true for the length field in the slice. Fixed.

@bors
Copy link
Contributor

bors commented Sep 17, 2015

⌛ Testing commit 6def06c with merge 2be0d0a...

bors added a commit that referenced this pull request Sep 17, 2015
Currently, we're generating adjustments, for example, to get from &[u8]
to &[u8], which is unneeded and kicks us out of trans_into()
into trans() which means an additional stack slot and copy in the
unoptimized code.
@bors bors merged commit 6def06c into rust-lang:master Sep 17, 2015
steveklabnik added a commit to steveklabnik/rust that referenced this pull request Sep 30, 2015
The original blog post referred to examples by their file names, and now
that it's in guide form, there is no file name. So edit the text so that
it makes a bit more sense.

Fixes rust-lang#28428
steveklabnik added a commit to steveklabnik/rust that referenced this pull request Sep 30, 2015
The original blog post referred to examples by their file names, and now
that it's in guide form, there is no file name. So edit the text so that
it makes a bit more sense.

Fixes rust-lang#28428
@dotdash dotdash deleted the same_adjust branch January 15, 2016 08:40
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.

4 participants