Skip to content

Option optimization: do not create redundant local vars #7915

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

Merged
merged 7 commits into from
Sep 22, 2025
Merged

Option optimization: do not create redundant local vars #7915

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
eb96936
Option optimization: do not create redundant local vars
cknitt Sep 20, 2025
a564d26
Cleanup
cknitt Sep 22, 2025
f336bda
Comment
cknitt Sep 22, 2025
66d9910
Address review comments
cknitt Sep 22, 2025
61f75df
Don't use Lam_analysis.no_side_effects here
cknitt Sep 22, 2025
cb98fbe
Document refine_let rewrite semantics
cristianoc Sep 22, 2025
418e733
CHANGELOG
cknitt Sep 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 55 additions & 88 deletions compiler/core/lam_util.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,86 +38,61 @@ let add_required_modules ( x : Ident.t list) (meta : Lam_stats.t) =
*)


(*
It's impossible to have a case like below:
{[
(let export_f = ... in export_f)
]}
Even so, it's still correct
*)
let refine_let
~kind param
(arg : Lam.t) (l : Lam.t) : Lam.t =

match (kind : Lam_compat.let_kind ), arg, l with
| _, _, Lvar w when Ident.same w param
(* let k = xx in k
there is no [rec] so [k] would not appear in [xx]
*)
-> arg (* TODO: optimize here -- it's safe to do substitution here *)
| _, _, Lprim {primitive ; args = [Lvar w]; loc ; _} when Ident.same w param
&& (function | Lam_primitive.Pmakeblock _ -> false | _ -> true) primitive
(* don't inline inside a block *)
-> Lam.prim ~primitive ~args:[arg] loc
(* we can not do this substitution when capttured *)
(* | _, Lvar _, _ -> (\** let u = h in xxx*\) *)
(* (\* assert false *\) *)
(* Ext_log.err "@[substitution >> @]@."; *)
(* let v= subst_lambda (Map_ident.singleton param arg ) l in *)
(* Ext_log.err "@[substitution << @]@."; *)
(* v *)
| _, _, Lapply {ap_func=fn; ap_args = [Lvar w]; ap_info; ap_transformed_jsx} when
Ident.same w param &&
(not (Lam_hit.hit_variable param fn ))
->
(* does not work for multiple args since
evaluation order unspecified, does not apply
for [js] in general, since the scope of js ir is loosen

here we remove the definition of [param]
{[ let k = v in (body) k
]}
#1667 make sure body does not hit k
*)
Lam.apply fn [arg] ap_info ~ap_transformed_jsx
| (Strict | StrictOpt ),
( Lvar _ | Lconst _ |
Lprim {primitive = Pfield (_ , Fld_module _) ;
args = [ Lglobal_module _ | Lvar _ ]; _}) , _ ->
(* (match arg with *)
(* | Lconst _ -> *)
(* Ext_log.err "@[%a %s@]@." *)
(* Ident.print param (string_of_lambda arg) *)
(* | _ -> ()); *)
(* No side effect and does not depend on store,
since function evaluation is always delayed
*)
Lam.let_ Alias param arg l
| ( (Strict | StrictOpt ) ), (Lfunction _ ), _ ->
(*It can be promoted to [Alias], however,
we don't want to do this, since we don't want the
function to be inlined to a block, for example
{[
let f = fun _ -> 1 in
[0, f]
]}
TODO: punish inliner to inline functions
into a block
*)
Lam.let_ StrictOpt param arg l
(* Not the case, the block itself can have side effects
we can apply [no_side_effects] pass
| Some Strict, Lprim(Pmakeblock (_,_,Immutable),_) ->
Llet(StrictOpt, param, arg, l)
*)
| Strict, _ ,_ when Lam_analysis.no_side_effects arg ->
Lam.let_ StrictOpt param arg l
| Variable, _, _ ->
Lam.let_ Variable param arg l
| kind, _, _ ->
Lam.let_ kind param arg l
(* | None , _, _ ->
Lam.let_ Strict param arg l *)
(* refine_let normalises let-bindings so we avoid redundant locals while
preserving evaluation semantics. *)
let refine_let ~kind param (arg : Lam.t) (l : Lam.t) : Lam.t =
let is_block_constructor = function
| Lam_primitive.Pmakeblock _ -> true
| _ -> false
in
let is_safe_to_inline (lam : Lam.t) =
Copy link
Collaborator

Choose a reason for hiding this comment

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

OK now this is a very special case of Lam_analysis.no_side_effects : why not just use Lam_analysis.no_side_effects . Unless we know that's wrong. I would just test it see what happens.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Specifically, Lam_analysis.no_side_effects implies is_safe_to_inline as the code is written today.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, if I change is_safe_to_inline to Lam_analysis.no_side_effects, I get many changes because functions are reordered in the output. But this one doesn't look too good:

function swapUnsafe(xs, i, j) {
  let tmp = xs[i];
  xs[i] = xs[j];
  xs[j] = tmp;
}

->

function swapUnsafe(xs, i, j) {
  xs[i] = xs[j];
  xs[j] = xs[i];
}

But this means that we shouldn't rely on Lam_analysis.no_side_effects for the payload of Psome_not_nest either.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK then I think the transformation needs to be explained in clear terms in the code.
Then each case needs to justify why it is OK (in comments) so it's evident that it's safe.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me add more detailed comments in a second.

match lam with
| Lvar _ | Lconst _ -> true
| Lprim { primitive = Psome_not_nest; args = [inner]; _ } ->
Lam_analysis.no_side_effects inner
| Lprim { primitive = Pfield (_, Fld_module _); args = [ (Lglobal_module _ | Lvar _) ]; _ } -> true
| _ -> false
in
match (kind : Lam_compat.let_kind), arg, l with
| _, _, Lvar w when Ident.same w param ->
(* If the body immediately returns the binding (e.g. `{ let x = value; x }`),
we skip creating `x` and keep `value`. There is no `rec`, so `value`
cannot refer back to `x`, and we avoid generating a redundant local. *)
arg
| _, _, Lprim { primitive; args = [ Lvar w ]; loc; _ }
when Ident.same w param && not (is_block_constructor primitive) ->
(* When we immediately feed the binding into a primitive, like
`{ let x = value; Array.length(x) }`, we inline the primitive call
with `value`. This only happens for primitives that are pure and do not
allocate new blocks, so evaluation order and side effects stay the same. *)
Lam.prim ~primitive ~args:[arg] loc
| _, _, Lapply { ap_func = fn; ap_args = [ Lvar w ]; ap_info; ap_transformed_jsx }
when Ident.same w param && not (Lam_hit.hit_variable param fn) ->
(* For a function call such as `{ let x = value; someFn(x) }`, we can
rewrite to `someFn(value)` as long as the callee does not capture `x`.
This removes the temporary binding while preserving the call semantics. *)
Lam.apply fn [arg] ap_info ~ap_transformed_jsx
| (Strict | StrictOpt), arg, _ when is_safe_to_inline arg ->
(* `Strict` and `StrictOpt` bindings both evaluate the RHS immediately
(with `StrictOpt` allowing later elimination if unused). When that RHS
is pure — `{ let x = Some(value); ... }`, `{ let x = 3; ... }`, or a module
field read — we mark it as an alias so downstream passes can inline the
original expression and drop the temporary. *)
Lam.let_ Alias param arg l
| Strict, Lfunction _, _ ->
(* If we eagerly evaluate a function binding such as
`{ let makeGreeting = () => "hi"; ... }`, we end up allocating the
closure immediately. Downgrading `Strict` to `StrictOpt` preserves the
original laziness while still letting later passes inline when safe. *)
Lam.let_ StrictOpt param arg l
| Strict, _, _ when Lam_analysis.no_side_effects arg ->
(* A strict binding whose expression has no side effects — think
`{ let x = computePure(); use(x); }` — can be relaxed to `StrictOpt`.
This keeps the original semantics yet allows downstream passes to skip
evaluating `x` when it turns out to be unused. *)
Lam.let_ StrictOpt param arg l
| kind, _, _ ->
Lam.let_ kind param arg l

let alias_ident_or_global (meta : Lam_stats.t) (k:Ident.t) (v:Ident.t)
(v_kind : Lam_id_kind.t) =
Expand Down Expand Up @@ -260,11 +235,3 @@ let is_var (lam : Lam.t) id =
lapply (let a = 3 in let b = 4 in fun x y -> x + y) 2 3
]}
*)








4 changes: 2 additions & 2 deletions tests/gentype_tests/typescript-react-example/src/nested/Types.res.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 2 additions & 5 deletions tests/tests/src/option_optimisation.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@
import * as Primitive_option from "@rescript/runtime/lib/es6/Primitive_option.js";

function boolean(val1, val2) {
let a = val1;
let b = val2;
if (b || a) {
if (val2 || val1) {
return "a";
} else {
return "b";
Expand Down Expand Up @@ -33,8 +31,7 @@ function constant() {
}

function param(opt) {
let x = opt;
console.log(x);
console.log(opt);
}

export {
Expand Down
28 changes: 14 additions & 14 deletions tests/tests/src/option_wrapping_test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,6 @@ let x6 = {
x: 42
};

let x7 = [
1,
2,
3
];

let x8 = () => {};

let x10 = null;

let x11 = Primitive_option.some(undefined);
Expand All @@ -24,8 +16,6 @@ let x20 = null;

let x21 = new Date();

let x22 = /test/;

let x23 = new Map();

let x24 = new Set();
Expand Down Expand Up @@ -58,8 +48,6 @@ let x37 = new Intl.DateTimeFormat();

let x38 = new Intl.NumberFormat();

let x39 = true;

let x40 = new Intl.Collator();

let x41 = new Intl.RelativeTimeFormat();
Expand All @@ -70,8 +58,6 @@ let x43 = new Intl.Locale("en");

let x45 = Promise.resolve(true);

let x47 = {};

let x48 = Stdlib_Lazy.make(() => true);

let x1 = "hello";
Expand All @@ -89,15 +75,29 @@ let x5 = {
x: 42
};

let x7 = [
1,
2,
3
];

let x8 = () => {};

let x12 = "test";

let x22 = /test/;

let x39 = true;

let x44 = [
1,
2
];

let x46 = /* [] */0;

let x47 = {};

export {
x1,
x2,
Expand Down
23 changes: 11 additions & 12 deletions tests/tests/src/reasonReact.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,17 +92,6 @@ function wrapReasonForJs(component, jsPropsToReason) {
let dummyInteropComponent = basicComponent("interop");

function wrapJsForReason(reactClass, props, children) {
let jsElementWrapped = (extra, extra$1) => {
let props$1 = Object.assign(Object.assign({}, props), {
ref: extra$1,
key: extra
});
let varargs = Js_array.concat(children, [
reactClass,
props$1
]);
return React.createElement.apply(null, varargs);
};
return {
debugName: dummyInteropComponent.debugName,
reactClassInternal: dummyInteropComponent.reactClassInternal,
Expand All @@ -117,7 +106,17 @@ function wrapJsForReason(reactClass, props, children) {
initialState: dummyInteropComponent.initialState,
retainedProps: dummyInteropComponent.retainedProps,
reducer: dummyInteropComponent.reducer,
jsElementWrapped: jsElementWrapped
jsElementWrapped: (extra, extra$1) => {
let props$1 = Object.assign(Object.assign({}, props), {
ref: extra$1,
key: extra
});
let varargs = Js_array.concat(children, [
reactClass,
props$1
]);
return React.createElement.apply(null, varargs);
}
};
}

Expand Down
Loading