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

inline fast path of caml_applyN #934

Merged
merged 7 commits into from
Jan 18, 2023
Merged

inline fast path of caml_applyN #934

merged 7 commits into from
Jan 18, 2023

Conversation

gretay-js
Copy link
Contributor

@gretay-js gretay-js commented Nov 1, 2022 •
edited
Loading

Add a flag -caml-apply-inline-fast-path and update cmm_helpers to emit an indirect call instead of a call to caml_apply if the number of arguments expected by the closure is the same as the number of arguments provided.

Improve performance of code that calls caml_applyN which has an unpredictable indirect call.

Gbury
Gbury requested changes Nov 1, 2022
View reviewed changes
backend/cmm_helpers.ml Outdated Show resolved Hide resolved
@gretay-js
Copy link
Contributor Author

This is ready for review.

CI build on "flambda2 macos" failed with an error that seems unrelated to this patch.

(cd _build/runtime_stdlib/ocaml/runtime && /usr/bin/make -sj8 libasmrun.a libasmrund.a libasmruni.a libasmrun_pic.a libasmrun_shared.so prims.o)
cp: primitives: Permission denied``

@mshinwell
Copy link
Collaborator 

(cd _build/runtime_stdlib/ocaml/runtime && /usr/bin/make -sj8 libasmrun.a libasmrund.a libasmruni.a libasmrun_pic.a libasmrun_shared.so prims.o)
cp: primitives: Permission denied``

This is some kind of race condition that's been seen before, I'll rerun the job

xclerc
xclerc reviewed Nov 2, 2022
View reviewed changes
backend/cmm_helpers.ml Show resolved Hide resolved
backend/cmm_helpers.ml Outdated Show resolved Hide resolved
@mshinwell
Copy link
Collaborator

I'm unclear why we need a command-line flag for this -- it seems a reasonable code size / performance tradeoff?

@gretay-js
Copy link
Contributor Author

I'm unclear why we need a command-line flag for this -- it seems a reasonable code size / performance tradeoff?

I'm not sure it is always reasonable. Maybe it should always be on by default with flambda and flambda2, but off with closure and -Oclassic.

@mshinwell
Copy link
Collaborator

I'm unclear why we need a command-line flag for this -- it seems a reasonable code size / performance tradeoff?

I'm not sure it is always reasonable. Maybe it should always be on by default with flambda and flambda2, but off with closure and -Oclassic.

That sounds good to me, then we shouldn't need a flag.

@lthls
Copy link
Contributor

lthls commented Nov 3, 2022

I'm not sure the code size/performance trade-off is that great. In particular, if you have a tight loop with one path, seldom used, that does an indirect call, you don't want to increase the size of the loop just to optimise the call.
If you have benchmarks that show a noticeable improvement, then I'm fine with letting this on by default, but I'm expecting regressions in a few cases.

@lpw25
Copy link
Collaborator

lpw25 commented Dec 5, 2022

I think we should have this behaviour under a flag first and then we can explore the trade-off in practice. We have internal people keen to try this out in some specific places, and I'd like to let them do that before we spend the effort working out what the right default is.

mshinwell
mshinwell approved these changes Jan 18, 2023
View reviewed changes
Copy link
Collaborator

@mshinwell mshinwell left a comment

Choose a reason for hiding this comment

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

Only files owned by me

@gretay-js
Copy link
Contributor Author

@Gbury I think I've addressed all your comments so far. Can you please approve this PR if you don't have any other comments?

xclerc
xclerc approved these changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@xclerc xclerc left a comment

Choose a reason for hiding this comment

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

As noted in a CR, this pull request makes the
fast case faster and the slow case slower.
I don't think it is blocking, but the developers
benchmarking this version should be aware
of that to be sure they don't get overall numbers
where the two effects are cancelling each other
out.

@gretay-js gretay-js merged commit bba0b9f into ocaml-flambda:main Jan 18, 2023
@smuenzel
Copy link
Contributor

I'm not exactly sure what assembly this currently generates, but it would be nice if the Cifthenelse (Ceq(Casr(Cload(),offset),arity)) was compiled to

cmp m8(byte of closure arity), imm8 (expected arity)
jne

I'm guessing that other that with a very specific pattern match for just this situation, this is currently beyond the capabilities of our instruction selection.
I would assume this would also help with code size, and with the same test in the caml_apply function.

@smuenzel
Copy link
Contributor

@gretay-js In case you want to test this suggestion, seems to generate correct code:
ocaml/ocaml@trunk...smuenzel:ocaml:Ispecifictest

ccasin added a commit to ccasin/flambda-backend that referenced this pull request Jan 30, 2023
@ccasin
Squashed 'ocaml/' changes from a09392d..5b98caf
1f1d430 
5b98caf flambda-backend: Revert "Restructure LIBDIR" (ocaml-flambda#1093)
46f1b16 flambda-backend: Remove a stray character from dynlink Makefile (ocaml-flambda#1090)
e515442 flambda-backend: Restructure LIBDIR: Move Dynlink, Str and Unix to sub-directories (port PR#11198) (ocaml-flambda#895)
9307e7c flambda-backend: inline fast path of caml_applyN (ocaml-flambda#934)
3244386 flambda-backend: Bump magic numbers (ocaml-flambda#1077)
7c9e15d flambda-backend: Fix bug in arguments to `get_unit_info` (ocaml-flambda#1069)
a2f4c9e flambda-backend: Closure rename static catch (ocaml-flambda#1070)
2ee705a flambda-backend: Backport ocaml-flambda#946 to ocaml/ subfolder (ocaml-flambda#1061)
569703f flambda-backend: Merge ocaml-jst#97 (ocaml-flambda#1063)
7b1779a flambda-backend: Port value_kind changes to testsuite/tools to fix parsecmm.mly (ocaml-flambda#1060)
49505d1 flambda-backend: Bugfix for Ctype.nondep_type (ocaml-flambda#1059)
654c63c flambda-backend: Backport ocaml-flambda#295 kind changes to ocaml/ subfolder (ocaml-flambda#1018)
50a9ce0 flambda-backend: Fix extern_closure_up_to_env (ocaml-flambda#1053)
bc2c78d flambda-backend: Remove obsolete pack-related code (ocaml-flambda#1051)
872ff38 flambda-backend: Fix error-prone syntax resulting from reformatting (ocaml-flambda#1050)
d03b346 flambda-backend: Fix different .cmi files being produced by ocamlopt and ocamlc when using -pack (ocaml-flambda#1049)
90ee37b flambda-backend: Revert "Revert "Use Import_info.t in Cmt_format"" (ocaml-flambda#1045)
ac12d90 flambda-backend: Fix version number (ocaml-flambda#1043)
75e0154 flambda-backend: Revert "Use Import_info.t in Cmt_format" (ocaml-flambda#1042)
ecab74c flambda-backend: Use Import_info.t in Cmt_format (ocaml-flambda#1037)
7661d4d flambda-backend: Bootstrap
1098a56 flambda-backend: Update .depend files
a7292da flambda-backend: Revert ocaml/toplevel/native/dune
7584479 flambda-backend: Remove alloc-check from ocaml/

git-subtree-dir: ocaml
git-subtree-split: 5b98caf
ccasin added a commit to ccasin/flambda-backend that referenced this pull request Mar 12, 2023
@ccasin
Squashed 'ocaml/' changes from c703f5f..77434f0
d93152b 
77434f0 flambda-backend: Bump magic numbers for 4.14.1-5 (ocaml-flambda#1190)
3a78e83 flambda-backend: Revert "Instance compilation units" (ocaml-flambda#1175)
9a683c9 flambda-backend: Move compute_layout to Lambda (ocaml-flambda#1167)
0ea58e9 flambda-backend: Revert "Add a proper top and bottom layout" (ocaml-flambda#1169)
1e5e23a flambda-backend: Add hint for common misplaced [@unboxed] attribute (ocaml-flambda#1164)
10f870a flambda-backend: Add a proper top and bottom layout (ocaml-flambda#1158)
d1be563 flambda-backend: Don't warn about misplaced attributes in -i mode (ocaml-flambda#1163)
feefcaa flambda-backend: Remove immutable arrays from stdlib.ml and stdlib.mli (ocaml-flambda#1154)
ba101be flambda-backend: Shrink the ref_table if it grows large (ocaml-flambda#1156)
777fda7 flambda-backend: Preserve backtraces from failing `Lazy_backtrack` computations (ocaml-flambda#805)
56d014e flambda-backend: Remove remaining `layout_top` after ocaml-flambda#1084 (ocaml-flambda#1138)
dc1c1ce flambda-backend: Layouts for parameters in lambda & remove most layout_top (ocaml-flambda#1084)
49fea78 flambda-backend: Instance compilation units (ocaml-flambda#1113)
1127fd2 flambda-backend: Merge pull request ocaml-flambda#1143 from riaqn/merge-ocaml-jst
f458733 flambda-backend: fix things after merge
b43d385 flambda-backend: File magic updates (ocaml-flambda#306)
9f604aa flambda-backend: Merge ocaml-jst
1f54613 flambda-backend: More assertions for local alloc and letrec (ocaml-flambda#1081)
0c95280 flambda-backend: Remove most layout_top in closure (ocaml-flambda#1127)
8ad48b5 flambda-backend: Use only `.ocamlformat*` to enable or disable ocamlformat (ocaml-flambda#1135)
1aa2885 flambda-backend: Build all boot targets in `make hacking` (ocaml-flambda#1133)
6b6c25a flambda-backend: Hide `Compilation_unit.t`'s definition in a submodule (ocaml-flambda#1134)
529d66b flambda-backend: Compile refactor (ocaml-flambda#1096)
df798c1 flambda-backend: Propagates layouts through Flambda1 (ocaml-flambda#1115)
9bce50b flambda-backend: Add `gc-timings` to collect timing information from the GC (ocaml-flambda#1089)
d431d3b flambda-backend: Use the type of primitive declarations to make their layout (ocaml-flambda#1118)
8da887e flambda-backend: Changes to arity in clambda (ocaml-flambda#1106)
db20e97 flambda-backend: Fix simplify-exits (ocaml-flambda#1108)
6a63906 flambda-backend: Add layout on Lregion (ocaml-flambda#1107)
c562fb3 flambda-backend: caml_{curry,apply,send}* for unboxed types (ocaml-flambda#1104)
50ee311 flambda-backend: Temporary fix for incorrect layout for result of custom and operators (ocaml-flambda#1119)
c89512d flambda-backend: More error checking for natdynlink symbols (ocaml-flambda#1005)
27d68bf flambda-backend: Flambda1 region deletion and locals fixes (ocaml-flambda#1000)
0e3e057 flambda-backend: Add identifiers for instantiated functor units (ocaml-flambda#1092)
4b75b46 flambda-backend: Fix build system under ocaml/runtime (ocaml-flambda#1085)
7c9fc32 flambda-backend: Fix valid character range computation for compilation units (ocaml-flambda#1110)
40c754a flambda-backend: Add result layout in Lapply and Lsend (ocaml-flambda#1102)
cedaea1 flambda-backend: Add layout type in Lambda (ocaml-flambda#1032)
47c0e23 flambda-backend: Configure: Add flag to use legacy library layout (ocaml-flambda#1098)
eed5888 flambda-backend: Restructure LIBDIR: Move Dynlink, Str and Unix to sub-directories (port part of ocaml/PR11198) (ocaml-flambda#1094)
5b98caf flambda-backend: Revert "Restructure LIBDIR" (ocaml-flambda#1093)
46f1b16 flambda-backend: Remove a stray character from dynlink Makefile (ocaml-flambda#1090)
e515442 flambda-backend: Restructure LIBDIR: Move Dynlink, Str and Unix to sub-directories (port PR#11198) (ocaml-flambda#895)
9307e7c flambda-backend: inline fast path of caml_applyN (ocaml-flambda#934)
3244386 flambda-backend: Bump magic numbers (ocaml-flambda#1077)
7c9e15d flambda-backend: Fix bug in arguments to `get_unit_info` (ocaml-flambda#1069)
a2f4c9e flambda-backend: Closure rename static catch (ocaml-flambda#1070)
2ee705a flambda-backend: Backport ocaml-flambda#946 to ocaml/ subfolder (ocaml-flambda#1061)
569703f flambda-backend: Merge ocaml-jst#97 (ocaml-flambda#1063)
7b1779a flambda-backend: Port value_kind changes to testsuite/tools to fix parsecmm.mly (ocaml-flambda#1060)
49505d1 flambda-backend: Bugfix for Ctype.nondep_type (ocaml-flambda#1059)
654c63c flambda-backend: Backport ocaml-flambda#295 kind changes to ocaml/ subfolder (ocaml-flambda#1018)
50a9ce0 flambda-backend: Fix extern_closure_up_to_env (ocaml-flambda#1053)
bc2c78d flambda-backend: Remove obsolete pack-related code (ocaml-flambda#1051)
872ff38 flambda-backend: Fix error-prone syntax resulting from reformatting (ocaml-flambda#1050)
d03b346 flambda-backend: Fix different .cmi files being produced by ocamlopt and ocamlc when using -pack (ocaml-flambda#1049)
90ee37b flambda-backend: Revert "Revert "Use Import_info.t in Cmt_format"" (ocaml-flambda#1045)
ac12d90 flambda-backend: Fix version number (ocaml-flambda#1043)
75e0154 flambda-backend: Revert "Use Import_info.t in Cmt_format" (ocaml-flambda#1042)
ecab74c flambda-backend: Use Import_info.t in Cmt_format (ocaml-flambda#1037)
7661d4d flambda-backend: Bootstrap
1098a56 flambda-backend: Update .depend files
a7292da flambda-backend: Revert ocaml/toplevel/native/dune
7584479 flambda-backend: Remove alloc-check from ocaml/

git-subtree-dir: ocaml
git-subtree-split: 77434f0
ccasin added a commit that referenced this pull request Mar 24, 2023
@ccasin
Squashed 'ocaml/' changes from c703f5f..bcae5ff
2376a0b 
bcae5ff flambda-backend: Bump buffer size to avoid gcc warning (#1247)
d6c170b flambda-backend: Remove `-absname` to improve dune build errors (#1233)
1b2bcb4 flambda-backend: Put frametables in .text with relative offsets for return addresses. (#1227)
999d523 flambda-backend: Small optimisation for caml_modify (#1226)
b16493b flambda-backend: Add timestamps to GC logs (#1229)
95f7e80 flambda-backend: Provide a no-naked-pointers runtime and use it for the compiler (#1224)
ba77581 flambda-backend: Replace assertion with a match statement (#1225)
cfb3cd2 flambda-backend: Memoise last substitution composition (#1209)
1c4db44 flambda-backend: Port PR1202 and PR1205 to the ocaml/ subtree (#1211)
577410e flambda-backend: Correctly stack debuginfo for inlined body in classic mode (#1152)
6733de6 flambda-backend: Fix dune install in otherlibs: missing cmt and typo. (#1194)
4c97d26 flambda-backend: Unboxed numbers (#1165)
1ad7252 flambda-backend: Revert "Revert "Add a proper top and bottom layout" (#1169)" (#1191)
dea4b3e flambda-backend: Don't get the layout of arguments from patterns (#1179)
6d3e85b flambda-backend: Don't copy when resolving aliases in try_modtypes (#1184)
73e52b7 flambda-backend: Clarify the types used for static jump/catch (#1180)
273a40d flambda-backend: Reenable backtrace testsuite folder for flambda2 (#1161)
77434f0 flambda-backend: Bump magic numbers for 4.14.1-5 (#1190)
3a78e83 flambda-backend: Revert "Instance compilation units" (#1175)
9a683c9 flambda-backend: Move compute_layout to Lambda (#1167)
0ea58e9 flambda-backend: Revert "Add a proper top and bottom layout" (#1169)
1e5e23a flambda-backend: Add hint for common misplaced [@unboxed] attribute (#1164)
10f870a flambda-backend: Add a proper top and bottom layout (#1158)
d1be563 flambda-backend: Don't warn about misplaced attributes in -i mode (#1163)
feefcaa flambda-backend: Remove immutable arrays from stdlib.ml and stdlib.mli (#1154)
ba101be flambda-backend: Shrink the ref_table if it grows large (#1156)
777fda7 flambda-backend: Preserve backtraces from failing `Lazy_backtrack` computations (#805)
56d014e flambda-backend: Remove remaining `layout_top` after #1084 (#1138)
dc1c1ce flambda-backend: Layouts for parameters in lambda & remove most layout_top (#1084)
49fea78 flambda-backend: Instance compilation units (#1113)
1127fd2 flambda-backend: Merge pull request #1143 from riaqn/merge-ocaml-jst
f458733 flambda-backend: fix things after merge
b43d385 flambda-backend: File magic updates (#306)
9f604aa flambda-backend: Merge ocaml-jst
1f54613 flambda-backend: More assertions for local alloc and letrec (#1081)
0c95280 flambda-backend: Remove most layout_top in closure (#1127)
8ad48b5 flambda-backend: Use only `.ocamlformat*` to enable or disable ocamlformat (#1135)
1aa2885 flambda-backend: Build all boot targets in `make hacking` (#1133)
6b6c25a flambda-backend: Hide `Compilation_unit.t`'s definition in a submodule (#1134)
529d66b flambda-backend: Compile refactor (#1096)
df798c1 flambda-backend: Propagates layouts through Flambda1 (#1115)
9bce50b flambda-backend: Add `gc-timings` to collect timing information from the GC (#1089)
d431d3b flambda-backend: Use the type of primitive declarations to make their layout (#1118)
8da887e flambda-backend: Changes to arity in clambda (#1106)
db20e97 flambda-backend: Fix simplify-exits (#1108)
6a63906 flambda-backend: Add layout on Lregion (#1107)
c562fb3 flambda-backend: caml_{curry,apply,send}* for unboxed types (#1104)
50ee311 flambda-backend: Temporary fix for incorrect layout for result of custom and operators (#1119)
c89512d flambda-backend: More error checking for natdynlink symbols (#1005)
27d68bf flambda-backend: Flambda1 region deletion and locals fixes (#1000)
0e3e057 flambda-backend: Add identifiers for instantiated functor units (#1092)
4b75b46 flambda-backend: Fix build system under ocaml/runtime (#1085)
7c9fc32 flambda-backend: Fix valid character range computation for compilation units (#1110)
40c754a flambda-backend: Add result layout in Lapply and Lsend (#1102)
cedaea1 flambda-backend: Add layout type in Lambda (#1032)
47c0e23 flambda-backend: Configure: Add flag to use legacy library layout (#1098)
eed5888 flambda-backend: Restructure LIBDIR: Move Dynlink, Str and Unix to sub-directories (port part of ocaml/PR11198) (#1094)
5b98caf flambda-backend: Revert "Restructure LIBDIR" (#1093)
46f1b16 flambda-backend: Remove a stray character from dynlink Makefile (#1090)
e515442 flambda-backend: Restructure LIBDIR: Move Dynlink, Str and Unix to sub-directories (port PR#11198) (#895)
9307e7c flambda-backend: inline fast path of caml_applyN (#934)
3244386 flambda-backend: Bump magic numbers (#1077)
7c9e15d flambda-backend: Fix bug in arguments to `get_unit_info` (#1069)
a2f4c9e flambda-backend: Closure rename static catch (#1070)
2ee705a flambda-backend: Backport #946 to ocaml/ subfolder (#1061)
569703f flambda-backend: Merge ocaml-jst#97 (#1063)
7b1779a flambda-backend: Port value_kind changes to testsuite/tools to fix parsecmm.mly (#1060)
49505d1 flambda-backend: Bugfix for Ctype.nondep_type (#1059)
654c63c flambda-backend: Backport #295 kind changes to ocaml/ subfolder (#1018)
50a9ce0 flambda-backend: Fix extern_closure_up_to_env (#1053)
bc2c78d flambda-backend: Remove obsolete pack-related code (#1051)
872ff38 flambda-backend: Fix error-prone syntax resulting from reformatting (#1050)
d03b346 flambda-backend: Fix different .cmi files being produced by ocamlopt and ocamlc when using -pack (#1049)
90ee37b flambda-backend: Revert "Revert "Use Import_info.t in Cmt_format"" (#1045)
ac12d90 flambda-backend: Fix version number (#1043)
75e0154 flambda-backend: Revert "Use Import_info.t in Cmt_format" (#1042)
ecab74c flambda-backend: Use Import_info.t in Cmt_format (#1037)
7661d4d flambda-backend: Bootstrap
1098a56 flambda-backend: Update .depend files
a7292da flambda-backend: Revert ocaml/toplevel/native/dune
7584479 flambda-backend: Remove alloc-check from ocaml/

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

@lthls lthls lthls left review comments

@Gbury Gbury Gbury approved these changes

@xclerc xclerc xclerc approved these changes

@mshinwell mshinwell mshinwell approved these changes

Assignees
No one assigned
Labels
backend
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gretay-js @mshinwell @lthls @lpw25 @smuenzel @xclerc @Gbury