Skip to content

Backend changes for multiple returns#1268

Merged
mshinwell merged 4 commits intooxcaml:mainfrom
lthls:multiple-return-backend-only
Apr 11, 2023
Merged

Backend changes for multiple returns#1268
mshinwell merged 4 commits intooxcaml:mainfrom
lthls:multiple-return-backend-only

Conversation

@lthls
Copy link
Copy Markdown
Contributor

@lthls lthls commented Mar 29, 2023

Extracted from #1248.
Two changes are not strictly necessary:

  • The patch to spill.ml is just an unrelated bug fix that I made while debugging the code.
  • The addition of Cmm_helpers.make_tuple is useful for generating Cmm code with multiple returns, but could be kept as part of the middle-end patches instead.

I chose to integrate them anyway because it was simpler to extract everything in the backend/ folder, and they're short and easy to review.

I haven't backported those changes to the ocaml/ directory, I couls probably do it as part of this PR too.

@lthls lthls requested review from gretay-js and xclerc as code owners March 29, 2023 12:52
@lthls lthls linked an issue Mar 29, 2023 that may be closed by this pull request
Support for multiple return values (not user-visible) #981
Open
@Ekdohibs Ekdohibs mentioned this pull request Apr 4, 2023
Merged
@lthls lthls force-pushed the multiple-return-backend-only branch from 8fdeef2 to fb2411d Compare April 6, 2023 12:24
gretay-js
gretay-js approved these changes Apr 10, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@gretay-js gretay-js left a comment

Choose a reason for hiding this comment

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

The changes in various Proc, Cmm_helpers and Selectgen all look fine to me. As far as I can see, the generated code will be the same as before for functions with a single return.

Do we expect to use unboxed types in the compiler itself? If not, I'd rather not make these changes to code generation ocaml/asmcomp directory, unless of course we need them for ocaml-jst.

@xclerc can you please confirm that the change in validator test is correct? I don't understand this test enough to be sure that it needs Proc.loc_results_return everywhere or Proc.loc_results_call.

The fix for spill.ml : I would prefer to put it into a separate PR. This bug might cause miscompilation, unless Iexit labels are not unique across different functions in the same compilation unit or somehow the spill instructions it returns are ignorexd. Wonder why we have never noticed this bug in the generated code.

@mshinwell
Copy link
Copy Markdown
Collaborator

mshinwell commented Apr 10, 2023

We've ported all of the middle-end layout changes to ocaml/ so I think we should probably do the same as this one, then ocaml-jst will be in a fully working state. Agreed that the fix to Spill should be in a separate PR.

gretay-js
gretay-js requested changes Apr 10, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@gretay-js gretay-js left a comment

Choose a reason for hiding this comment

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

  • split out the bug fix in spill
  • port proc changes to asmcomp

@xclerc
Copy link
Copy Markdown
Contributor

xclerc commented Apr 10, 2023

@xclerc can you please confirm that the change in validator test is correct? I don't understand this test enough to be sure that it needs Proc.loc_results_return everywhere or Proc.loc_results_call.

The change look correct to me, but I
am a bit worried it is not tested.

@lthls
Copy link
Copy Markdown
Contributor Author

lthls commented Apr 11, 2023

I've split out the spill change (will submit a separate PR later), and ported the changes to the ocaml/ folder, including all supported architectures.
I get an error for riscv when running make check_all_arches, but it seems unrelated to this PR. All the other architectures build correctly.

@mshinwell mshinwell merged commit 566249e into oxcaml:main Apr 11, 2023
mshinwell added a commit to mshinwell/oxcaml that referenced this pull request Apr 28, 2023
@mshinwell
Squashed 'ocaml/' changes from ea89813..a7d005a
4d0fa29 
a7d005a flambda-backend: Lazy deserialization of cmi files (oxcaml#1322)
aa83fa3 flambda-backend: Reinstate previous API for Env.lookup_value (oxcaml#1323)
e4007a4 flambda-backend: Lazy substitution into value_declaration (oxcaml#1320)
634b607 flambda-backend: Merge Types.* and Subst.Lazy.* types (oxcaml#1312)
cf82708 flambda-backend: Bump magic numbers for 4.14.1-7 (oxcaml#1317)
6470400 flambda-backend: zero_alloc attribute payload "assert all" and "ignore" (oxcaml#1296)
bba5248 flambda-backend: Teach `ocamldep` about all the language extensions (oxcaml#1303)
33e97b0 flambda-backend: Change Includemod to work on lazy modtypes (oxcaml#1228)
16e5002 flambda-backend: zero_alloc new warning for unchecked functions (oxcaml#1302)
36b4626 flambda-backend: Attribute [@@@zero_alloc check] to turn the check on (oxcaml#1294)
3b524c6 flambda-backend: Cmm.value_kind cleanup (oxcaml#1091)
ec99505 flambda-backend: Fix failure of `check_all_arches` on RISCV (oxcaml#1300)
450bc58 flambda-backend: Backend changes for multiple returns (oxcaml#1268)
84a7a26 flambda-backend: Static check for zero_alloc: ignore allocation that lead to exceptional return (oxcaml#1157)
1723728 flambda-backend: Re-enable parallel build of the runtime (oxcaml#1287)
26ea7f3 flambda-backend: Fix closure marshalling when not in NNP mode (oxcaml#1286)
9b91f2e flambda-backend: Reduce number of caml_apply functions taking/returning "I" and "V" (oxcaml#1272)
1686928 flambda-backend: Restore Cmm unboxing behaviour inside regions (oxcaml#1285)
cf9be42 flambda-backend: Fix all the no-naked-pointers problems (oxcaml#1282)
8fe089e flambda-backend: Unrevert oxcaml#1131 and fix a Cmm unboxing bug (oxcaml#1284)
c4143c3 flambda-backend: Revert "Make Selectgen treat region boundaries more precisely" (oxcaml#1283)
2078dce flambda-backend: Add some -dtimings output for the typechecker (oxcaml#1245)
273a7f9 flambda-backend: Make Selectgen treat region boundaries more precisely (oxcaml#1131)
47610e6 flambda-backend: Bump magic numbers for 4.14.1-6 (oxcaml#1274)
fd53d38 flambda-backend: Generate *.cms files for merlin (oxcaml#1232)
853f95f flambda-backend: Add tail_mod_const to builtin_attrs (oxcaml#1265)
f9ef051 flambda-backend: Fix issue caused by effects of gadt expansion in mode cross check (oxcaml#1263)
e9ffcf8 flambda-backend: Fix dependencies for regenerating Flambda2 parser, tests (oxcaml#1255)
6f1cd1f flambda-backend: Restore a lost location, needed for merlin (oxcaml#1242)
009332b flambda-backend: Fix merge from ocaml-jst

git-subtree-dir: ocaml
git-subtree-split: a7d005a
ccasin added a commit to ccasin/oxcaml that referenced this pull request Apr 29, 2023
@ccasin
Squashed 'ocaml/' changes from ea89813..a7d005a
35bb136 
a7d005a flambda-backend: Lazy deserialization of cmi files (oxcaml#1322)
aa83fa3 flambda-backend: Reinstate previous API for Env.lookup_value (oxcaml#1323)
e4007a4 flambda-backend: Lazy substitution into value_declaration (oxcaml#1320)
634b607 flambda-backend: Merge Types.* and Subst.Lazy.* types (oxcaml#1312)
cf82708 flambda-backend: Bump magic numbers for 4.14.1-7 (oxcaml#1317)
6470400 flambda-backend: zero_alloc attribute payload "assert all" and "ignore" (oxcaml#1296)
bba5248 flambda-backend: Teach `ocamldep` about all the language extensions (oxcaml#1303)
33e97b0 flambda-backend: Change Includemod to work on lazy modtypes (oxcaml#1228)
16e5002 flambda-backend: zero_alloc new warning for unchecked functions (oxcaml#1302)
36b4626 flambda-backend: Attribute [@@@zero_alloc check] to turn the check on (oxcaml#1294)
3b524c6 flambda-backend: Cmm.value_kind cleanup (oxcaml#1091)
ec99505 flambda-backend: Fix failure of `check_all_arches` on RISCV (oxcaml#1300)
450bc58 flambda-backend: Backend changes for multiple returns (oxcaml#1268)
84a7a26 flambda-backend: Static check for zero_alloc: ignore allocation that lead to exceptional return (oxcaml#1157)
1723728 flambda-backend: Re-enable parallel build of the runtime (oxcaml#1287)
26ea7f3 flambda-backend: Fix closure marshalling when not in NNP mode (oxcaml#1286)
9b91f2e flambda-backend: Reduce number of caml_apply functions taking/returning "I" and "V" (oxcaml#1272)
1686928 flambda-backend: Restore Cmm unboxing behaviour inside regions (oxcaml#1285)
cf9be42 flambda-backend: Fix all the no-naked-pointers problems (oxcaml#1282)
8fe089e flambda-backend: Unrevert oxcaml#1131 and fix a Cmm unboxing bug (oxcaml#1284)
c4143c3 flambda-backend: Revert "Make Selectgen treat region boundaries more precisely" (oxcaml#1283)
2078dce flambda-backend: Add some -dtimings output for the typechecker (oxcaml#1245)
273a7f9 flambda-backend: Make Selectgen treat region boundaries more precisely (oxcaml#1131)
47610e6 flambda-backend: Bump magic numbers for 4.14.1-6 (oxcaml#1274)
fd53d38 flambda-backend: Generate *.cms files for merlin (oxcaml#1232)
853f95f flambda-backend: Add tail_mod_const to builtin_attrs (oxcaml#1265)
f9ef051 flambda-backend: Fix issue caused by effects of gadt expansion in mode cross check (oxcaml#1263)
e9ffcf8 flambda-backend: Fix dependencies for regenerating Flambda2 parser, tests (oxcaml#1255)
6f1cd1f flambda-backend: Restore a lost location, needed for merlin (oxcaml#1242)
009332b flambda-backend: Fix merge from ocaml-jst

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

Reviewers

@Gbury Gbury Gbury approved these changes

@gretay-js gretay-js gretay-js approved these changes

@xclerc xclerc Awaiting requested review from xclerc

Assignees

No one assigned

Labels

backend unboxed types

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support for multiple return values (not user-visible)

5 participants

@lthls @mshinwell @xclerc @Gbury @gretay-js