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

Linearize: for Trywith, remove the jump/call to the handler #2237

Merged
merged 5 commits into from Mar 7, 2019

Conversation

@lthls
Copy link
Contributor

commented Feb 6, 2019

The previous instruction now falls through the body, with the code for the handler after the body.
It leads to some performance gains on x86 architectures because the extra call instruction interfered with the processor's return predictor, leading to branch misses.

This pull request was built from a patch by @gretay-js for amd64, with the code for the other architectures extracted from #1482. Some extensive testing had been done for #1482, but this PR has not been tested on actual non-x86 systems yet.

@gretay-js

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

@lthls I'll have a look at it.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

@lthls Maybe you could send this to precheck?

@gretay-js

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

The changes in amd64,i386,arm64,arm look fine to me. I don't know enough about the other targets to be confident.
I wonder about the choice of temporary registers for use in the code emitted for Lraise in amd64 (r11) and i386 (ebx). Is there any particular reason for choosing these registers specifically, and if so - perhaps it should be documented.

@lthls

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

I wonder about the choice of temporary registers for use in the code emitted for Lraise in amd64 (r11) and i386 (ebx). Is there any particular reason for choosing these registers specifically, and if so - perhaps it should be documented.

The raise instruction isn't expected to preserve any general purpose register (except the one containing the exception). So any general purpose register can be used here, I don't remember if there was any particular reason for using these ones specifically.

@gretay-js

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

ok, thanks.

@lthls lthls force-pushed the lthls:swap_trywith branch from a831cb8 to 002dbbf Mar 5, 2019
@lthls lthls force-pushed the lthls:swap_trywith branch from 002dbbf to 404c6a3 Mar 5, 2019
@lthls

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

This PR was sent to precheck (see here, thanks @gretay-js), and it looks like it doesn't break anything. I consider this ready for merging.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

I will cast an eye over this before approving it. @gretay-js is going to resolve the CR in the amd64 backend first, however.

Copy link
Contributor

left a comment

It passes all the test. I'll remove the CR.

asmcomp/amd64/emit.mlp Outdated Show resolved Hide resolved
asmcomp/linearize.ml Outdated Show resolved Hide resolved
asmcomp/linearize.mli Outdated Show resolved Hide resolved
asmcomp/arm/emit.mlp Outdated Show resolved Hide resolved
@lthls

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

The issues from the review should have been addressed.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

I've looked through this again with @gretay-js and we didn't notice anything wrong. Merging.

@mshinwell mshinwell merged commit 1dba532 into ocaml:trunk Mar 7, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
xavierleroy added a commit that referenced this pull request Apr 3, 2019
The addresses of Thumb-2 exception handlers are computed incorrectly following the merge of #2237.

The fix is to make sure that the bottom bit of the address is set when in Thumb-2 mode,
Closes: #8578
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.