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

Fix a bunch of warnings (fixes #565) #566

Merged
merged 2 commits into from
Aug 6, 2016
Merged

Conversation

nox
Copy link
Contributor

@nox nox commented Aug 5, 2016

No description provided.

@eddyb
Copy link
Member

eddyb commented Aug 6, 2016

I don't like this, it's not trivial to optimize like the current version. The only thing you can do better is dispatch to another macro or 4 impls on arrays of sizes 0 through 3.

@nox
Copy link
Contributor Author

nox commented Aug 6, 2016

"It's not trivial to optimise" doesn't actually say anything about whether it is optimised or not, which sounds like FUD to me.

@eddyb
Copy link
Member

eddyb commented Aug 6, 2016

@nox As discussed on IRC, it depends on loop unrolling, which is a tradeoff between speed and code size.
LLVM has heuristics for it which I don't know, but I believe atypical leading/trailing iterations may trigger it.

If you want to generate a memcpy, don't we have a copy_from method on slices?

@nox nox force-pushed the wall-of-warnings branch from 49dbe88 to 8cfd763 Compare August 6, 2016 14:41
@nox
Copy link
Contributor Author

nox commented Aug 6, 2016

Amended with copy_from, thanks @eddyb!

@nox nox force-pushed the wall-of-warnings branch from 8cfd763 to 6e6a7a7 Compare August 6, 2016 14:44
@eddyb
Copy link
Member

eddyb commented Aug 6, 2016

Wow, look at the Release mode LLVM IR or assembly for before and after this PR.
copy_from_slice (wink wink @ubsan) wins by being a single mov eax, dword ptr [rdi] instruction.

cc @pcwalton This looks like one of those oddities you've seen while reading Rust ASM. MIR trans doesn't help at all here, seems that LLVM just gives up on making it simple for itself.

@eddyb
Copy link
Member

eddyb commented Aug 6, 2016

Oddly enough, running the worst one though opt again produces the best one. There might be a LLVM pass ordering issue.

@eddyb eddyb merged commit d79d156 into image-rs:master Aug 6, 2016
@nox nox deleted the wall-of-warnings branch August 10, 2016 10:26
@eddyb
Copy link
Member

eddyb commented Aug 15, 2016

Looks like the super-ugly IR for the for_loop function in the testcase (that opt cleans up) may have a common root with rust-lang/rust#35662 (pointer ranges being optimized worse in LLVM 3.9).

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.

2 participants