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.

Sign up for GitHub

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

Jump to bottom

Backport ocaml/ocaml#10595 from upstream/trunk #471

Merged
merged 4 commits into from Feb 8, 2022
Merged

Backport ocaml/ocaml#10595 from upstream/trunk #471

merged 4 commits into from Feb 8, 2022

Conversation

Gbury
Copy link
Contributor

@Gbury Gbury commented Jan 18, 2022 •
edited

Backport ocaml/ocaml#10595 that raises the max number of arguments for a tailcall.

I had to manually edit files to apply the diff from the original PR because the diff could not be applied cleanly. Indeed, a lof of changes to the backend in ocaml/ocaml have not been backported yet. Most notably, there are two PR in trunk that also affect the same locations, but are not currently part of the ocaml tree in flambda-backend:

I wasn't quite sure what to do with the Changes part of the diff, @mshinwell what's the policy on that part ?

I think a careful review from @lthls is required to make sure I haven't made any typo (considering he reviewed the original PR).

@Gbury Gbury requested a review from lthls January 18, 2022 15:50
@Gbury
Copy link
Contributor Author

Gbury commented Jan 18, 2022

Oh, so we're in the very unusual case where flambda2 succeeds, but flambda1 and closure fail, I'll try and look at what's happening.

@Gbury
Copy link
Contributor Author

Gbury commented Jan 18, 2022

CI is fixed, this is ready for review.

@mshinwell
Copy link
Collaborator

@xclerc do you think you could review this please? (Background: some of the Flambda 2 simplifier has been made non-tail-recursive by accident due to functions with lots of arguments, and this seems like a good patch to have anyway.)

@xclerc
Copy link
Contributor

xclerc commented Feb 1, 2022

Sure, I will review it today; sorry for the delay.

xclerc
xclerc approved these changes Feb 1, 2022
View reviewed changes
Copy link
Contributor

@xclerc xclerc left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two remarks (both fairly orthogonal to this pull request):

  • the tests in this pull request are not executed on
    this repository for flambda2 since they are in a
    directory either not opted-in or opted-opt (depending
    on which side of Flambda2 test list #494 you live);
  • it is not clear we need to keep ocaml/asmcomp/xyz
    when xyz is neither amd64 nor arm64.

@Gbury
Backport PR#10595 from upstream/trunk
4a87669 
Note: PR#10595 from upstream/trunk does not apply cleanly,
considering that a lof of changes to the backend have not been
backported yet. Most notably, there are two PR that also changes
the same files:
- PR#8936 factored away most of the global mutable state used in
  emit.mlp files into an env passed to a lot of functions. Luckily
  it does not interact with these changes (apart from creating
  annoying conflicts in diffs)
- PR#10578 raised the number of registers used on s390x and power,
  (thus altering the max_arguments_for_tailcalls and
   some calls to "calling_conventions" in the `proc.ml` files)
@Gbury
Also apply the diff to files in backend/
a615a28
@Gbury
Fix partial match in reg.ml
0d6f491
@Gbury
Enable tests from the basic upstream directory
852d0a3
@mshinwell mshinwell merged commit 5631ad5 into ocaml-flambda:main Feb 8, 2022
mshinwell pushed a commit that referenced this pull request Mar 3, 2022
@Gbury @mshinwell
Backport ocaml/ocaml#10595 from upstream/trunk (#471)
0dd90d4
stedolan added a commit to stedolan/flambda-backend that referenced this pull request Mar 7, 2022
@stedolan
Squashed 'ocaml/' changes from 173842ce84..64235a382a
c6bb9c5 
64235a382a flambda-backend: Change Float.nan from sNaN to qNaN (ocaml-flambda#466)
14a8e27063 flambda-backend: Track GC work for all managed bigarray allocations (upstream 11022) (ocaml-flambda#569)
c3cda96154 flambda-backend: Add two new methods to targetint for dwarf (ocaml-flambda#560)
e6f1fed2f5 flambda-backend: Handle arithmetic overflow in select_addr (ocaml-flambda#570)
dab7209a12 flambda-backend: Add Target_system to ocaml/utils (ocaml-flambda#542)
82d5044871 flambda-backend: Enhance numbers.ml with more primitive types (ocaml-flambda#544)
216be99334 flambda-backend: Fix flambda_o3 and flambda_oclassic attributes (ocaml-flambda#536)
4b56e07c1d flambda-backend: Test naked pointer root handling (ocaml-flambda#550)
40d69cef86 flambda-backend: Stop local function optimisation from moving code into function bodies; opaque_identity fixes for class compilation (ocaml-flambda#537)
f08ae5851c flambda-backend: Implemented inlining history and use it inside inlining reports (ocaml-flambda#365)
ac496bf52e flambda-backend: Disable the local keyword in typing (ocaml-flambda#540)
7d46712f7a flambda-backend: Bugfix for Typedtree generation of arrow types (ocaml-flambda#539)
61a7b47773 flambda-backend: Insert missing page table check in roots_nat.c (ocaml-flambda#541)
323bd36d98 flambda-backend: Compiler error when -disable-all-extensions and -extension are used (ocaml-flambda#534)
d8956b09e4 flambda-backend: Persistent environment and reproducibility (ocaml-flambda#533)
4a0c89f117 flambda-backend: Revert "Revert bswap PRs (480 and 482)" (ocaml-flambda#506)
7803705828 flambda-backend: Cause a C warning when CAMLreturn is missing in C stubs. (ocaml-flambda#376)
6199db5b26 flambda-backend: Improve unboxing during cmm for Flambda (ocaml-flambda#295)
96b9e1ba6d flambda-backend: Print diagnostics at runtime for Invalid (ocaml-flambda#530)
42ab88e8a1 flambda-backend: Disable bytecode compilers in ocamltest (ocaml-flambda#504)
58c72d5476 flambda-backend: Backport ocaml/ocaml#10595 from upstream/trunk (ocaml-flambda#471)
10105394de flambda-backend: Use C++ name mangling convention (ocaml-flambda#483)
81881bbf88 flambda-backend: Local allocation test no longer relies on lifting (ocaml-flambda#525)
f5c47190f6 flambda-backend: Fix an assertion in Closure that breaks probes (ocaml-flambda#505)
c2cf2b2a14 flambda-backend: Add some missing command line arguments to ocamlnat (ocaml-flambda#499)

git-subtree-dir: ocaml
git-subtree-split: 64235a382a0424cced40eed328ddf1dfb9645f87
stedolan added a commit that referenced this pull request Mar 7, 2022
@stedolan
Squashed 'ocaml/' changes from 173842ce84..64235a382a
b54d3a3 
64235a382a flambda-backend: Change Float.nan from sNaN to qNaN (#466)
14a8e27063 flambda-backend: Track GC work for all managed bigarray allocations (upstream 11022) (#569)
c3cda96154 flambda-backend: Add two new methods to targetint for dwarf (#560)
e6f1fed2f5 flambda-backend: Handle arithmetic overflow in select_addr (#570)
dab7209a12 flambda-backend: Add Target_system to ocaml/utils (#542)
82d5044871 flambda-backend: Enhance numbers.ml with more primitive types (#544)
216be99334 flambda-backend: Fix flambda_o3 and flambda_oclassic attributes (#536)
4b56e07c1d flambda-backend: Test naked pointer root handling (#550)
40d69cef86 flambda-backend: Stop local function optimisation from moving code into function bodies; opaque_identity fixes for class compilation (#537)
f08ae5851c flambda-backend: Implemented inlining history and use it inside inlining reports (#365)
ac496bf52e flambda-backend: Disable the local keyword in typing (#540)
7d46712f7a flambda-backend: Bugfix for Typedtree generation of arrow types (#539)
61a7b47773 flambda-backend: Insert missing page table check in roots_nat.c (#541)
323bd36d98 flambda-backend: Compiler error when -disable-all-extensions and -extension are used (#534)
d8956b09e4 flambda-backend: Persistent environment and reproducibility (#533)
4a0c89f117 flambda-backend: Revert "Revert bswap PRs (480 and 482)" (#506)
7803705828 flambda-backend: Cause a C warning when CAMLreturn is missing in C stubs. (#376)
6199db5b26 flambda-backend: Improve unboxing during cmm for Flambda (#295)
96b9e1ba6d flambda-backend: Print diagnostics at runtime for Invalid (#530)
42ab88e8a1 flambda-backend: Disable bytecode compilers in ocamltest (#504)
58c72d5476 flambda-backend: Backport ocaml/ocaml#10595 from upstream/trunk (#471)
10105394de flambda-backend: Use C++ name mangling convention (#483)
81881bbf88 flambda-backend: Local allocation test no longer relies on lifting (#525)
f5c47190f6 flambda-backend: Fix an assertion in Closure that breaks probes (#505)
c2cf2b2a14 flambda-backend: Add some missing command line arguments to ocamlnat (#499)

git-subtree-dir: ocaml
git-subtree-split: 64235a382a0424cced40eed328ddf1dfb9645f87
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xclerc xclerc xclerc approved these changes

@gretay-js gretay-js Awaiting requested review from gretay-js

@mshinwell mshinwell Awaiting requested review from mshinwell

@stedolan stedolan Awaiting requested review from stedolan

@lthls lthls Awaiting requested review from lthls

Assignees
No one assigned
Labels
backend backport from upstream OCaml
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Gbury @mshinwell @xclerc