amd64: Emit 32bit registers for Iconst_int when we can#8990
Merged
damiendoligez merged 1 commit intoocaml:trunkfrom Oct 3, 2019
Merged
Conversation
Contributor
Author
|
It's been pointed out that this particular change is one part of the proposed changes in #1490. |
xavierleroy
approved these changes
Sep 27, 2019
Contributor
xavierleroy
left a comment
There was a problem hiding this comment.
I'm in favor. As I wrote in #1490:
Using
movl $n, %R32instead ofmovq $n, %R64when n is in [0...0xFFFF_FFFF] is a clear win: two bytes saved, very common idiom, no risk of performance regression.
1131aaa to
9bc1113
Compare
Contributor
Author
|
I've spoken with @xclerc and @mshinwell. Taken their (better) code in #1490 for emitting 32bit registers for constants. |
xavierleroy
approved these changes
Sep 27, 2019
Contributor
xavierleroy
left a comment
There was a problem hiding this comment.
Getting even better. Style suggestion below.
…se of (better) code proposed in PR1490 credit to xclerc/mshinwell)
9bc1113 to
cf112de
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR alters how
Iconst_intis emitted for x86_64 to use a more compact encoding.Currently
Iconst_intwill emit:The Intel optimization reference manual recommends using 32bit registers (see section 12.2.1). This PR changes to emit:
Taking ocamlopt as the binary, this change reduces the text segment size by about 0.75%. I have also run sandmark benchmarks for the change, there was no significant evidence to suggest performance gains or losses. However as a comparison GCC, Clang and ICC are all emitting the 32bit variants.