-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Stop Simplif
from causing disagreement between runtime arity and syntatic arity of a function
#12496
Stop Simplif
from causing disagreement between runtime arity and syntatic arity of a function
#12496
Conversation
Tests from: - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391) - ocaml/ocaml#12496 (not merged)
Tests from: - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391) - ocaml/ocaml#12496 (not merged)
Tests from: - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391) - ocaml/ocaml#12496 (not merged)
Tests from: - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391) - ocaml/ocaml#12496 (not merged)
Tests from: - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391) - ocaml/ocaml#12496 (not merged)
Tests from: - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391) - ocaml/ocaml#12496 (not merged)
Tests from: - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391) - ocaml/ocaml#12496 (not merged)
Tests from: - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391) - ocaml/ocaml#12496 (not merged)
Tests from: - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391) - ocaml/ocaml#12496 (not merged)
I'm not a huge fan of using allocations to test arity. let runtime_arity (f: _ -> _) =
let clos_info = Obj.raw_field (Obj.repr f) 1 in
let raw_arity =
Nativeint.(to_int (shift_right clos_info (Sys.word_size - 8)))
in
if raw_arity < 0 then (* tupled function *) 1 else raw_arity Do you think you could rewrite the test to use that instead ? |
50f6b77
to
ad63761
Compare
@lthls That seems reasonable enough. I've made that change. I force-pushed it so that any reviewer can still confirm that the test changes output as expected due to the code change. |
Co-authored-by: Richard Eisenberg <rae@richarde.dev>
…-roberts/ocaml into maintain-syntactic-arity-in-simplif
Tests from: - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391) - ocaml/ocaml#12496 (not merged)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ready to merge from my perspective. Does this need any more review before pulling the trigger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test file ended up more complex than I expected, but I'm fine with it (modulo a comment that I think is slightly wrong).
I have also looked at the PR itself and haven't found anything suspicious.
(* The "nested arity" of a value is either: | ||
- the empty list, if the value isn't a function | ||
- x :: xs if the value is a function [f], where [x] is [f]'s arity, and | ||
[xs] is the nested arity of the result of applying [f] to a value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When reading this sentence, it looks like fun _ _ -> 0
would have nested arity [Curried 2; Curried 1]
(for some values of 2 and 1).
But I think what you actually compute is the expected [Curried 2]
. Maybe we could replace to a value
by to the number of values corresponding to its arity
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the correction.
* Newtypes * Constraint/coercion * Add map_half_typed_cases * Implement type-checking/translation This also promotes tests whose output changes. * Add upstream tests Tests from: - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391) - ocaml/ocaml#12496 (not merged) * Fix ocamldoc * Update chamelon minimizer * Respond to requested changes to minimizer * update new test brought in from rebase * Fix bug in chunking code * `make bootstrap` * Add Ast_invariant check * Fix type-directed disambiguation of optional arg defaults * Minor comments from review * Run syntactic-arity test, update output, and fix printing bug * Remove unnecessary call to escape * Backport changes from upstream to comparative alloc tests * Avoid the confusing [Split_function_ty] module * Comment [split_function_ty] better. * [contains_gadt] as variant instead of bool * Calculate is_final_val_param on the fly rather than precomputing indexes * Note suboptimality * Get typecore typechecking * Finish resolving merge conflicts and run tests * make bootstrap * Add iteration on / mapping over locations and attributes * Reduce diff and fix typo in comment: * promote change to zero-alloc arg structure * Undo unintentional formatting changes to chamelon * Fix minimizer * Minimize diff * Fix bug with local-returning method * Fix regression where polymorphic parameters weren't allowed to be used in same parameter list as GADTs * Fix merge conflicts and make bootstrap * Apply expected diff to zero-alloc test changed in this PR
A stated goal of the Syntactic Arity RFC (ocaml/RFCs#32) was for runtime arity to match syntactic arity1. The implementation of syntactic arity in #12236 does not fully implement that goal (unintentionally). For example, even the motivating
make_greeter
example given in the RFC currently compiles as a function with runtime arity 3, though the spec given in the RFC suggests its runtime arity should match its syntactic arity of 2. This isn't a question of correctness but rather a question of performance: calling a function with runtime arity 2 is optimized at application sites to 2 syntactic arguments.This PR fixes the implementation to match the RFC.
The piece we missed in #12236:
Simplif
looks for nestedlfunction
s and fuses their parameter lists. In order for runtime arity to match syntactic arity, this rewrite shouldn't happen for functions that correspond tofun
/function
terms that the user wrote.Functors
Notably, this PR does not change how
Simplif
's parameter fusion rewrite applies to other constructs that compile to functions, like functors and class functions. The arity of these constructs are out of the scope of the original syntactic arity RFC and PR (though perhaps one day we'll want to follow the RFC's model for these constructs as well). Furthermore, basic experimentation shows that multi-argument functors rely onSimplif
's rewrite to be given the "expected" arity; e.g.module Make (X1 : S) (X2 : S) = ...
is only given an arity of 2 by virtue of this rewrite and would otherwise be a 1-ary function returning a 1-ary function.If it's decided that it's not important to preserve this rewrite for functors/class functions, it would simplify this PR to just delete
Simplif
's rewrite instead of the current approach of exceptingfun
/function
terms.Review
To review this PR, I'd suggest reading the two commits in order. The first commit adds a regression test demonstrating the discrepancy, and the second commit fixes it and updates the test output.
Footnotes
"Function definitions in OCaml are curried by default, in principle accepting one argument at a time and returning a closure accepting the rest. Since most function applications are saturated (that is, they pass the same number of arguments as the function definition expects), the implementation of functions is optimised to make saturated applications fast. [...] However, this optimisation requires knowing how many arguments a function definition expects. Since this information is not currently tracked in the parse tree, it must be guessed later, and sometimes this guess is wrong. [...] N-ary lambdas would make this issue go away, because the number of parameters of a function is known rather than inferred." ↩