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

Avoid using load/stores on first class aggregates #38906

Closed
wants to merge 1 commit into from

Conversation

dotdash
Copy link
Contributor

@dotdash dotdash commented Jan 7, 2017

LLVM usually prefers using memcpys over direct loads/store of first
class aggregates. The check in type_is_immediate to mark certain small
structs was originally part of the code to handle such immediates in
function arguments, and it had a counterpart in load_ty/store_ty to
actually convert small aggregates to integers.

But since then, the ABI handling has been refactored and takes care of
converting small aggregates to integers. During that refactoring, the
code to handle small aggregates in load_ty/store_ty has been removed,
and so we accidentally started using loads/stores on FCA values.

Since type_is_immediate() is no longer responsible for ABI-related
conversions, and using a memcpy even for small aggregates is usually
better than performing a FCA load/store, we can remove that code part
and only handle simple types as immediates there.

LLVM usually prefers using memcpys over direct loads/store of first
class aggregates. The check in type_is_immediate to mark certain small
structs was originally part of the code to handle such immediates in
function arguments, and it had a counterpart in load_ty/store_ty to
actually convert small aggregates to integers.

But since then, the ABI handling has been refactored and takes care of
converting small aggregates to integers. During that refactoring, the
code to handle small aggregates in load_ty/store_ty has been removed,
and so we accidentally started using loads/stores on FCA values.

Since type_is_immediate() is no longer responsible for ABI-related
conversions, and using a memcpy even for small aggregates is usually
better than performing a FCA load/store, we can remove that code part
and only handle simple types as immediates there.
@rust-highfive rust-highfive assigned pnkfelix and unassigned eddyb Jan 7, 2017
@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@arielb1
Copy link
Contributor

arielb1 commented Jan 7, 2017

Is there a measurable effect?

@eddyb
Copy link
Member

eddyb commented Jan 7, 2017

I'd like to combine this with #38854 somehow, but otherwise LGTM.

@dotdash
Copy link
Contributor Author

dotdash commented Jan 7, 2017

@arielb1 Compile times seem unaffected (checked a few crates from rust itself), and for libsyntax, unoptimized BC is a bit larger (1%), while optimized BC is a tiny bit smaller, but at the same time more code was inlined, so it seems like a win overall.

@sanxiyn
Copy link
Member

sanxiyn commented Jan 12, 2017

Travis failures seem legit.

@Mark-Simulacrum
Copy link
Member

I think tests passed on my local branch which included this, will update my PR (#38854) as soon as I have a chance.

@dotdash
Copy link
Contributor Author

dotdash commented Jan 12, 2017

Closing in favor of #38854 which includes my commit

@dotdash dotdash closed this Jan 12, 2017
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this pull request Jan 13, 2017
LLVM usually prefers using memcpys over direct loads/store of first
class aggregates. The check in type_is_immediate to mark certain small
structs was originally part of the code to handle such immediates in
function arguments, and it had a counterpart in load_ty/store_ty to
actually convert small aggregates to integers.

But since then, the ABI handling has been refactored and takes care of
converting small aggregates to integers. During that refactoring, the
code to handle small aggregates in load_ty/store_ty has been removed,
and so we accidentally started using loads/stores on FCA values.

Since type_is_immediate() is no longer responsible for ABI-related
conversions, and using a memcpy even for small aggregates is usually
better than performing a FCA load/store, we can remove that code part
and only handle simple types as immediates there.

This integrates PR rust-lang#38906 onto this branch.

Fixes rust-lang#38906.
@dotdash dotdash deleted the fcafix branch February 6, 2018 08:50
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.

None yet

7 participants