Skip to content

Ensure push_defaults can push past module patterns#11727

Merged
gasche merged 2 commits intoocaml:trunkfrom
stedolan:push-defaults-module-pat-fix
Nov 16, 2022
Merged

Ensure push_defaults can push past module patterns#11727
gasche merged 2 commits intoocaml:trunkfrom
stedolan:push-defaults-module-pat-fix

Conversation

@stedolan
Copy link
Contributor

@stedolan stedolan commented Nov 14, 2022

This patch fixes a small bug introduced in #10340 in the handling of first class module parameters. This is a performance issue rather than a correctness one: in the current code, module parameters defeat currying optimisations, so a function like the following will process arguments and allocate closures one at a time:

let f (module M : A) () = 1

The issue is that, while Translcore.push_defaults does contain code to specifically handle this case, it cannot be reached because the module binding expands to a pattern involving Tpat_alias, which fails the new is_trivial check. The fix is to extend is_trivial to cover Tpat_alias as well.

gasche
gasche approved these changes Nov 14, 2022
View reviewed changes
Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

Clearly correct.

Would you maybe consider making the catch-all case in trivial_pattern list the ignored constructors? It could have been a way to avoid this issue in the first place.

This may deserve a Changes entry (it fixes a performance bug that users may care about).

@stedolan
Copy link
Contributor Author

stedolan commented Nov 16, 2022

Thanks for the review. Changes changed.

Would you maybe consider making the catch-all case in trivial_pattern list the ignored constructors? It could have been a way to avoid this issue in the first place.

I don't actually think that would help here. We did think of and discuss handling other cases in is_trivial at the time, but this issue never came up. The problem is that is_trivial must not only handle code that the user writes, but also the fake parsetree nodes generated by expansion of module patterns. I think the ways to avoid this issue are (a) a test (which this patch adds), and (b) a less implicit encoding of module patterns (which I'd like to do someday, but not today)

@gasche gasche merged commit 6d68843 into ocaml:trunk Nov 16, 2022
stedolan added a commit to stedolan/flambda-backend that referenced this pull request Nov 17, 2022
@stedolan
Port ocaml/ocaml#11727 and ocaml/ocaml#11732
185ef98
stedolan added a commit to stedolan/flambda-backend that referenced this pull request Nov 17, 2022
@stedolan
Port ocaml/ocaml#11727 and ocaml/ocaml#11732
799a212
lpw25 pushed a commit to oxcaml/oxcaml that referenced this pull request Nov 18, 2022
@stedolan
Port ocaml/ocaml#11727 and ocaml/ocaml#11732 (#965)
7a6fe00
mshinwell pushed a commit to ocaml-flambda/ocaml-jst that referenced this pull request Dec 13, 2022
@stedolan
flambda-backend: Port ocaml/ocaml#11727 and ocaml/ocaml#11732 (#965)
8fb75bd
mshinwell added a commit to mshinwell/oxcaml that referenced this pull request Dec 13, 2022
@mshinwell
Squashed 'ocaml/' changes from 5bf2820..06c189a
76de3ce 
06c189a flambda-backend: Make stack allocation the default (oxcaml#1013)
98debd5 flambda-backend: Initial support for value slots not of value kind (oxcaml#946)
deb1714 flambda-backend: Add is_last flag to closinfo words (oxcaml#938)
d07fce1 flambda-backend: Disable poll insertion in Configure (oxcaml#967)
0f1ce0e flambda-backend: Regenerate ocaml/configure autoconf 2.69 (instead of 2.71) (oxcaml#1012)
27132d8 flambda-backend: Fix for spurious typing error related to expanding through functor arguments (oxcaml#997)
724fb68 flambda-backend: Use `Compilation_unit.t` instead of `Ident.t` for globals (oxcaml#871)
396d5b8 flambda-backend: Add a test for frametable setup in natdynlinked libraries (oxcaml#983)
b73ab12 flambda-backend: Fix invocation of `caml_shared_startup` in native dynlink (oxcaml#980)
7c7d75a flambda-backend: Fix split_default_wrapper which did not trigger anymore with flambda2 (oxcaml#970)
8fb75bd flambda-backend: Port ocaml/ocaml#11727 and ocaml/ocaml#11732 (oxcaml#965)
fdb7987 flambda-backend: Fix include functor issue after 4.14 merge. (oxcaml#948)
9745cdb flambda-backend: Print -dprofile/-dtimings output to stdout like 4.12 (oxcaml#943)
5f51f21 flambda-backend: Merge pull request oxcaml#932 from mshinwell/4.14-upgrade
841687d flambda-backend: Run make alldepend in ocaml/ (oxcaml#936)
72a7658 flambda-backend: Remove reformatting changes only in dynlink/dune (preserving PR889 and adjusting to minimise diff)
6d758cd flambda-backend: Revert whitespace changes in dune files, to match upstream
c86bf6e flambda-backend: Remove duplicate tests for polling
971dbeb flambda-backend: Testsuite fixes
32f8356 flambda-backend: Topeval fix for symbols patch
befea01 flambda-backend: Compilation fixes / rectify merge faults
a84543f flambda-backend: Merge ocaml-jst
8e65056 flambda-backend: Merge ocaml-jst
4d70045 flambda-backend: Remove filename from system frametable (amd64) (oxcaml#920)
5e57b7d flambda-backend: Bugfix for runtime frame_descr logic for C frames (oxcaml#918)
6423d5e flambda-backend: Merge pull request oxcaml#914 from mshinwell/merge-ocaml-jst-2022-10-24
ead605c flambda-backend: Add a missing Extract_exception (oxcaml#916)
c8f1481 flambda-backend: Resolve conflicts and add specialise/specialised attributes to Builtin_attributes
cf4d0d3 flambda-backend: Merge fixes (#21)
c2f742f flambda-backend: Re-enable some tests for Flambda2 (oxcaml#881)
3d38d13 flambda-backend: Long frames in frametable (oxcaml#797)
85aec7b flambda-backend: Add loop attribute to Builtin_attributes
c0f16e3 flambda-backend: Compilation fixes
90dea23 flambda-backend: Merge flambda-backend/main
5acc6ea flambda-backend: Fixes after merge
e501946 flambda-backend: Merge ocaml-jst
115083b flambda-backend: Merge ocaml-jst
9943b2e flambda-backend: Revert "Revert "Transform tail-recursive functions into recursive continuations (oxcaml#893)"" (oxcaml#909)
ce339f1 flambda-backend: Fix alloc modes and call kinds for overapplications (oxcaml#902)
e6a317c flambda-backend: Revert "Transform tail-recursive functions into recursive continuations (oxcaml#893)"
853c488 flambda-backend: Transform tail-recursive functions into recursive continuations (oxcaml#893)
5a977e4 flambda-backend: Fix missing End_region primitives on switch arms (oxcaml#898)
7fa7f9d flambda-backend: Add missing dependencies to Dune files (oxcaml#889)
3cd36f0 flambda-backend: Have Lambda `Pgetglobal` and `Psetglobal` take `Compilation_unit.t` (oxcaml#896)
7565915 flambda-backend: [@poll error] attribute (oxcaml#745)
9eb9448 flambda-backend: Backport the main safepoints PRs (oxcaml#740)
689bdda flambda-backend: Add strict mode for ocamldep (oxcaml#892)

git-subtree-dir: ocaml
git-subtree-split: 06c189a
mshinwell added a commit to mshinwell/oxcaml that referenced this pull request Dec 13, 2022
@mshinwell
Squashed 'ocaml/' changes from 5bf2820..06c189a
b273eee 
06c189a flambda-backend: Make stack allocation the default (oxcaml#1013)
98debd5 flambda-backend: Initial support for value slots not of value kind (oxcaml#946)
deb1714 flambda-backend: Add is_last flag to closinfo words (oxcaml#938)
d07fce1 flambda-backend: Disable poll insertion in Configure (oxcaml#967)
0f1ce0e flambda-backend: Regenerate ocaml/configure autoconf 2.69 (instead of 2.71) (oxcaml#1012)
27132d8 flambda-backend: Fix for spurious typing error related to expanding through functor arguments (oxcaml#997)
724fb68 flambda-backend: Use `Compilation_unit.t` instead of `Ident.t` for globals (oxcaml#871)
396d5b8 flambda-backend: Add a test for frametable setup in natdynlinked libraries (oxcaml#983)
b73ab12 flambda-backend: Fix invocation of `caml_shared_startup` in native dynlink (oxcaml#980)
7c7d75a flambda-backend: Fix split_default_wrapper which did not trigger anymore with flambda2 (oxcaml#970)
8fb75bd flambda-backend: Port ocaml/ocaml#11727 and ocaml/ocaml#11732 (oxcaml#965)
fdb7987 flambda-backend: Fix include functor issue after 4.14 merge. (oxcaml#948)
9745cdb flambda-backend: Print -dprofile/-dtimings output to stdout like 4.12 (oxcaml#943)
5f51f21 flambda-backend: Merge pull request oxcaml#932 from mshinwell/4.14-upgrade
841687d flambda-backend: Run make alldepend in ocaml/ (oxcaml#936)
72a7658 flambda-backend: Remove reformatting changes only in dynlink/dune (preserving PR889 and adjusting to minimise diff)
6d758cd flambda-backend: Revert whitespace changes in dune files, to match upstream
c86bf6e flambda-backend: Remove duplicate tests for polling
971dbeb flambda-backend: Testsuite fixes
32f8356 flambda-backend: Topeval fix for symbols patch
befea01 flambda-backend: Compilation fixes / rectify merge faults
a84543f flambda-backend: Merge ocaml-jst
8e65056 flambda-backend: Merge ocaml-jst
4d70045 flambda-backend: Remove filename from system frametable (amd64) (oxcaml#920)
5e57b7d flambda-backend: Bugfix for runtime frame_descr logic for C frames (oxcaml#918)
6423d5e flambda-backend: Merge pull request oxcaml#914 from mshinwell/merge-ocaml-jst-2022-10-24
ead605c flambda-backend: Add a missing Extract_exception (oxcaml#916)
c8f1481 flambda-backend: Resolve conflicts and add specialise/specialised attributes to Builtin_attributes
cf4d0d3 flambda-backend: Merge fixes (#21)
c2f742f flambda-backend: Re-enable some tests for Flambda2 (oxcaml#881)
3d38d13 flambda-backend: Long frames in frametable (oxcaml#797)
85aec7b flambda-backend: Add loop attribute to Builtin_attributes
c0f16e3 flambda-backend: Compilation fixes
90dea23 flambda-backend: Merge flambda-backend/main
5acc6ea flambda-backend: Fixes after merge
e501946 flambda-backend: Merge ocaml-jst
115083b flambda-backend: Merge ocaml-jst
9943b2e flambda-backend: Revert "Revert "Transform tail-recursive functions into recursive continuations (oxcaml#893)"" (oxcaml#909)
ce339f1 flambda-backend: Fix alloc modes and call kinds for overapplications (oxcaml#902)
e6a317c flambda-backend: Revert "Transform tail-recursive functions into recursive continuations (oxcaml#893)"
853c488 flambda-backend: Transform tail-recursive functions into recursive continuations (oxcaml#893)
5a977e4 flambda-backend: Fix missing End_region primitives on switch arms (oxcaml#898)
7fa7f9d flambda-backend: Add missing dependencies to Dune files (oxcaml#889)
3cd36f0 flambda-backend: Have Lambda `Pgetglobal` and `Psetglobal` take `Compilation_unit.t` (oxcaml#896)
7565915 flambda-backend: [@poll error] attribute (oxcaml#745)
9eb9448 flambda-backend: Backport the main safepoints PRs (oxcaml#740)
689bdda flambda-backend: Add strict mode for ocamldep (oxcaml#892)

git-subtree-dir: ocaml
git-subtree-split: 06c189a
mshinwell added a commit to mshinwell/oxcaml that referenced this pull request Dec 28, 2022
@mshinwell
Squashed 'ocaml/' changes from 5bf2820..b11eea1
f97a773 
b11eea1 flambda-backend: Introduce Import_info (oxcaml#1036)
bc5b135 flambda-backend: Fix `ocamlobjinfo` on flambda2 .cmx files (oxcaml#1029)
c8babbd flambda-backend: Compilation_unit optimisations (oxcaml#1035)
e8d3e22 flambda-backend: Use 4.14.0 opam switch for building (includes upgrading ocamlformat to 0.24.1) (oxcaml#1030)
eb14a86 flambda-backend: Port PR81 from ocaml-jst (oxcaml#1024)
131bc12 flambda-backend: Merge ocaml-jst 2022-12-13 (oxcaml#1022)
06c189a flambda-backend: Make stack allocation the default (oxcaml#1013)
98debd5 flambda-backend: Initial support for value slots not of value kind (oxcaml#946)
deb1714 flambda-backend: Add is_last flag to closinfo words (oxcaml#938)
d07fce1 flambda-backend: Disable poll insertion in Configure (oxcaml#967)
0f1ce0e flambda-backend: Regenerate ocaml/configure autoconf 2.69 (instead of 2.71) (oxcaml#1012)
27132d8 flambda-backend: Fix for spurious typing error related to expanding through functor arguments (oxcaml#997)
724fb68 flambda-backend: Use `Compilation_unit.t` instead of `Ident.t` for globals (oxcaml#871)
396d5b8 flambda-backend: Add a test for frametable setup in natdynlinked libraries (oxcaml#983)
b73ab12 flambda-backend: Fix invocation of `caml_shared_startup` in native dynlink (oxcaml#980)
7c7d75a flambda-backend: Fix split_default_wrapper which did not trigger anymore with flambda2 (oxcaml#970)
8fb75bd flambda-backend: Port ocaml/ocaml#11727 and ocaml/ocaml#11732 (oxcaml#965)
fdb7987 flambda-backend: Fix include functor issue after 4.14 merge. (oxcaml#948)
9745cdb flambda-backend: Print -dprofile/-dtimings output to stdout like 4.12 (oxcaml#943)
5f51f21 flambda-backend: Merge pull request oxcaml#932 from mshinwell/4.14-upgrade
841687d flambda-backend: Run make alldepend in ocaml/ (oxcaml#936)
72a7658 flambda-backend: Remove reformatting changes only in dynlink/dune (preserving PR889 and adjusting to minimise diff)
6d758cd flambda-backend: Revert whitespace changes in dune files, to match upstream
c86bf6e flambda-backend: Remove duplicate tests for polling
971dbeb flambda-backend: Testsuite fixes
32f8356 flambda-backend: Topeval fix for symbols patch
befea01 flambda-backend: Compilation fixes / rectify merge faults
a84543f flambda-backend: Merge ocaml-jst
8e65056 flambda-backend: Merge ocaml-jst
4d70045 flambda-backend: Remove filename from system frametable (amd64) (oxcaml#920)
5e57b7d flambda-backend: Bugfix for runtime frame_descr logic for C frames (oxcaml#918)
6423d5e flambda-backend: Merge pull request oxcaml#914 from mshinwell/merge-ocaml-jst-2022-10-24
ead605c flambda-backend: Add a missing Extract_exception (oxcaml#916)
c8f1481 flambda-backend: Resolve conflicts and add specialise/specialised attributes to Builtin_attributes
cf4d0d3 flambda-backend: Merge fixes (#21)
c2f742f flambda-backend: Re-enable some tests for Flambda2 (oxcaml#881)
3d38d13 flambda-backend: Long frames in frametable (oxcaml#797)
85aec7b flambda-backend: Add loop attribute to Builtin_attributes
c0f16e3 flambda-backend: Compilation fixes
90dea23 flambda-backend: Merge flambda-backend/main
5acc6ea flambda-backend: Fixes after merge
e501946 flambda-backend: Merge ocaml-jst
115083b flambda-backend: Merge ocaml-jst
9943b2e flambda-backend: Revert "Revert "Transform tail-recursive functions into recursive continuations (oxcaml#893)"" (oxcaml#909)
ce339f1 flambda-backend: Fix alloc modes and call kinds for overapplications (oxcaml#902)
e6a317c flambda-backend: Revert "Transform tail-recursive functions into recursive continuations (oxcaml#893)"
853c488 flambda-backend: Transform tail-recursive functions into recursive continuations (oxcaml#893)
5a977e4 flambda-backend: Fix missing End_region primitives on switch arms (oxcaml#898)
7fa7f9d flambda-backend: Add missing dependencies to Dune files (oxcaml#889)
3cd36f0 flambda-backend: Have Lambda `Pgetglobal` and `Psetglobal` take `Compilation_unit.t` (oxcaml#896)
7565915 flambda-backend: [@poll error] attribute (oxcaml#745)
9eb9448 flambda-backend: Backport the main safepoints PRs (oxcaml#740)
689bdda flambda-backend: Add strict mode for ocamldep (oxcaml#892)

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

Reviewers

@gasche gasche gasche approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@stedolan @gasche