Option optimization: do not create redundant local vars

[cknitt]

Made option temporaries created by the compiler’s “non-nested some” helper collapse into simple aliases, so the optimizer no longer leaves behind redundant local variables.

[pkg-pr-new]

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7915

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7915

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7915

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7915

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7915

@rescript/runtime

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/runtime@7915

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7915

commit: 418e733

[cristianoc]

I don't understand. "does not have side effects" is enough of a description? If so isn't there a function already.
Otherwise, let's keep it as is.

[cristianoc]

Btw some_not_nest could have side effects in the expression inside right?

[cknitt]

Yes, good catch, updated

[cknitt]

Oh no whitespace changes all over the file, one moment

[cknitt]

Ok should be fine now

[cristianoc]

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.

[cristianoc]

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

[cknitt]

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.

[cristianoc]

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.

[cristianoc]

Let me add more detailed comments in a second.

[cristianoc]

@cknitt see comments I've added.
Good to go for me.

[cristianoc]

We keep SafeAlias deliberately narrow because the (Alias) rewrite allows downstream codegen to inline the RHS or drop the const binding entirely. A pure-but-store-dependent read like let Strict x = array.(0) in array.(0) <- 1; x would pass Lam_analysis.no_side_effects, yet aliasing it would rewrite the block to array.(0) <- 1; array.(0) and observe the mutated value. Limiting the predicate to store-invariant forms (vars, constants, module fields, and Some wrappers thereof) keeps the semantics intact.

[cristianoc]

For reference, here is a fully spelled out is_store_invariant predicate, including the doc comment that states the contract explicitly. Each branch mirrors the semantics documented for the Alias rewrite.

(** [is_store_invariant lam] holds when re-evaluating [lam] after arbitrary
    heap updates yields the same value as the original eager evaluation.  This
    is the property required before upgrading a [Strict] or [StrictOpt] binding
    to [Alias]. *)
let rec is_store_invariant ?(env = Ident.Set.empty) (lam : Lam.t) : bool =
  let all l = Ext_list.for_all l (is_store_invariant ~env) in
  match lam with
  | Lconst _ -> true
  | Lvar v -> Ident.Set.mem v env
  | Lglobal_module _ -> true
  | Lfunction _ -> false
  | Lprim { primitive; args; _ } -> (
      match primitive, args with
      | ( Paddint | Psubint | Pmulint | Pnegint | Pnotint | Pandint | Porint
        | Pxorint | Plslint | Plsrint | Pasrint | Pintcomp _ | Pintorder
        | Pintmin | Pintmax
        | Pintoffloat | Pfloatofint | Pnegfloat | Paddfloat | Psubfloat
        | Pmulfloat | Pdivfloat | Pmodfloat | Ppowfloat | Pfloatcomp _
        | Pfloatorder | Pfloatmin | Pfloatmax
        | Pnegbigint | Paddbigint | Psubbigint | Pmulbigint | Pdivbigint
        | Pmodbigint | Ppowbigint | Pnotbigint | Pandbigint | Porbigint
        | Pxorbigint | Plslbigint | Pasrbigint | Pbigintcomp _
        | Pbigintorder | Pbigintmin | Pbigintmax
        | Pstringlength | Pstringrefu | Pstringrefs | Pstringcomp _
        | Pstringorder | Pstringmin | Pstringmax
        | Pboolcomp _ | Pboolmin | Pboolmax | Psequand | Psequor | Pnot
        | Pobjorder | Pobjmin | Pobjmax | Pobjtag | Pobjsize | Phash
        | Phash_mixstring | Phash_mixint | Phash_finalmix
        | Ptypeof | Pis_null | Pis_not_none | Pis_undefined | Pis_null_undefined
        | Pnull_to_opt | Pnull_undefined_to_opt | Pval_from_option
        | Pval_from_option_not_nest | Psome | Psome_not_nest | Pfn_arity ), _ ->
          all args
      | Pfield (_, Fld_module _), [ (Lglobal_module _ | Lvar _) as base ] ->
          is_store_invariant ~env base
      | Praw_js_code {code_info = Exp (Js_literal _ | Js_function _); _}, _ ->
          all args
      | _ -> false)
  | Llet (kind, id, rhs, body) ->
      let rhs_inv = is_store_invariant ~env rhs in
      let env' =
        if rhs_inv && (kind = Alias || kind = StrictOpt || kind = Strict) then
          Ident.Set.add id env
        else env
      in
      rhs_inv && is_store_invariant ~env:env' body
  | Lletrec _ -> false
  | Lsequence (a, b) -> is_store_invariant ~env a && is_store_invariant ~env b
  | Lifthenelse (c, t, e) ->
      is_store_invariant ~env c
      && is_store_invariant ~env t
      && is_store_invariant ~env e
  | Lswitch (scrut, cases) ->
      is_store_invariant ~env scrut
      && Ext_list.for_all_snd cases.sw_consts (is_store_invariant ~env)
      && Ext_list.for_all_snd cases.sw_blocks (is_store_invariant ~env)
      && Ext_option.for_all cases.sw_failaction (is_store_invariant ~env)
  | Lstringswitch (scrut, branches, default) ->
      is_store_invariant ~env scrut
      && Ext_list.for_all_snd branches (is_store_invariant ~env)
      && Ext_option.for_all default (is_store_invariant ~env)
  | Lstaticraise (_, args) -> all args
  | Lstaticcatch (body, _, handler) ->
      is_store_invariant ~env body && is_store_invariant ~env handler
  | Lapply _ -> false
  | Ltrywith (body, _, handler) ->
      is_store_invariant ~env body && is_store_invariant ~env handler
  | Lwhile _ | Lfor _ | Lassign _ -> false