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

Fixes for out-of-range Ialloc #1271

Merged
merged 2 commits into from Jul 31, 2017

Conversation

Projects
None yet
2 participants
@mshinwell
Copy link
Contributor

commented Jul 31, 2017

This should fix the underlying problem in #1250. When a closure is larger than the maximum permitted size for a minor heap value it is now allocated on the major heap instead.

I couldn't see any other places in Cmmgen that might suffer from a similar problem. I added an assertion in Selectgen for good measure. This patch also marginally improves debug info for closures.

@xavierleroy
Copy link
Contributor

left a comment

Looks good to me. Thanks!

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2017

Hmm, the test case takes far too long under Flambda by the look of it. I'll reduce the number of functions (it's also a bit slow when not using Flambda as well).

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2017

I was afraid of long compilation times for this test, indeed.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2017

I'll record my thoughts here although they should probably go elsewhere.

Some of the current code emitters start to emit syntactically incorrect assembly code for Ialloc past a certain allocation size, which is:

  • amd64: 2^31 bytes
  • arm: no limit
  • arm64: 2^12
  • i386: no limit
  • power: 2^15
  • s390x: 2^19

With Max_young_wosize = 256 as of today, none of those limits can be reached, so no action (of the kind considered in #1250) is necessary on the code emitters.

This said, this value of Max_young_wosize is quite low compared with the default minor heap size of 262144 words. We could increase it, in which case the arm64 port is the one that needs fixing along the lines of #1250.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2017

@xavierleroy Perhaps you could merge this, the tests have passed ok now.

@xavierleroy xavierleroy merged commit f689768 into ocaml:trunk Jul 31, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2017

Merged! And cherry-picked on the 4.05 branch (commit 664f076).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.