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.

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

Packed first-class module allocation is not hoisted to top-level #2867

Open
ncik-roberts opened this issue Jul 30, 2024 · 0 comments
Open

Packed first-class module allocation is not hoisted to top-level #2867

ncik-roberts opened this issue Jul 30, 2024 · 0 comments
Labels
bug Something isn't working flambda2 Prerequisite for, or part of, flambda2

Comments

@ncik-roberts
Copy link
Contributor

ncik-roberts commented Jul 30, 2024

Repro:

(* a.ml *)
module Make (T : sig val x : int ref end) = struct
  module L1 = struct
    let x = 3
    let y = T.x
  end
end

include Make [@inlined never] (struct
  let x = ref 1
end)
(* b.ml *)
module type S = sig
  module L1 : sig
    val y : int ref
  end
end

let run () = Sys.opaque_identity (module A : S)

Compile ocamlopt a.ml b.ml -dflambda (with or without -O3). The result is that run allocates the packed first-class module each time:

   (λ〈k32〉《k31》⟅my_region/88N⟆
      (param/90UV ∷ 𝕍=tagged_ℕ𝕚) (my_closure/89N ∷ 𝕍) my_depth/87N .
    Pfield/91N =
     (((Block_load (Values (tag ⊤) (size ⊤) (field_kind Any_value))
       Immutable) A.camlA 1) b.ml:7,41--42)
    Pfield/92N =
     (((Block_load (Values (tag ⊤) (size ⊤) (field_kind Any_value))
       Immutable) Pfield/91N 1) b.ml:7,41--42)
    Pmakeblock/93N =
     (((Make_block (Values (tag 0) (shape (𝕍))) Immutable Heap) Pfield/92N)
      b.ml:7,41--42)
    Pmakeblock/94N =
     (((Make_block (Values (tag 0) (shape (𝕍))) Immutable Heap)
       Pmakeblock/93N) b.ml:7,41--42)
    Popaque/95N =
     (((Opaque_identity (middle_end_only false) (kind 𝕍)) Pmakeblock/94N)
      b.ml:7,13--47)
    return k32 Popaque/95N)))

The expected behavior is that the packed module is hoisted to a top-level allocation.

This test is fairly sensitive to a lot of things:

  • S has to be a restriction on A's inferred signature; it can't be exactly A's inferred signature
  • S has to involve a nested module (here, L1) that with a binding that's some mutable value (here, an int ref).
@ncik-roberts ncik-roberts added flambda2 Prerequisite for, or part of, flambda2 bug Something isn't working labels Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working flambda2 Prerequisite for, or part of, flambda2
Projects
None yet
Development

No branches or pull requests

1 participant