Static allocation / simplification for unboxed product arrays #3363

mshinwell · 2024-12-11T16:38:46Z

This allows arrays involving unboxed products to be statically allocated and in certain cases have their accesses simplified, just like for normal arrays. This doesn't currently add to the testsuite; we can do that once #3317 has been merged with the augmented tests. Previously for testing I used:

(* ... with the temporary used to communicate to the C runtime stub
   being constant, and statically allocated *)

let make_unboxed_tuple_vect_scannable () =
  make_unboxed_tuple_vect 42 #(1, 2.0, "3")

let make_unboxed_tuple_vect_ignorable () =
  make_unboxed_tuple_vect 42 #(#1.0, 2, #3L, true)

let _ = make_unboxed_tuple_vect_scannable ()
let _ = make_unboxed_tuple_vect_ignorable ()

(* ... with the temporary used to communicate to the C runtime stub
   being inconstant, and statically allocated *)

let[@inline] make_unboxed_tuple_vect_scannable_inconstant () =
  make_unboxed_tuple_vect 42 #(Sys.opaque_identity 1, 2.0, "3")

let[@inline] make_unboxed_tuple_vect_ignorable_inconstant () =
  make_unboxed_tuple_vect 42 #(#1.0, (Random.int [@inlined never]) 42, #3L, true)

let _ = make_unboxed_tuple_vect_scannable_inconstant ()
let _ = make_unboxed_tuple_vect_ignorable_inconstant ()

(* ... with the temporary used to communicate to the C runtime stub
   being dynamically allocated on the local stack. *)

let[@inline never] make_unboxed_tuple_vect_scannable_dynamic () =
  make_unboxed_tuple_vect 42 #(Sys.opaque_identity 1, 2.0, "3")

let[@inline never] make_unboxed_tuple_vect_ignorable_dynamic () =
  make_unboxed_tuple_vect 42 #(#1.0, (Random.int [@inlined never]) 42, #3L, true)

let _ = make_unboxed_tuple_vect_scannable_dynamic ()
let _ = make_unboxed_tuple_vect_ignorable_dynamic ()

lthls · 2024-12-13T15:04:38Z

middle_end/flambda2/types/provers.ml

+    | [element_kind] -> (
+      match K.With_subkind.kind element_kind with
+      | Value -> Proved false
+      | Naked_number Naked_float -> Proved true


That looks wrong. If you look at the comment in flambda_primitive.mli, on the Unboxed_product constructor of the Array_kind.t type, it says that arrays of unboxed products are never float arrays (in the sense of having Double_array_tag). So because here we can't distinguish between a legacy flat float array and an array of unboxed floats represented as unboxed products of size 1, we risk returning a wrong answer.

Agreed, but the frontend will rule out unboxed products of size 1 (#3317 (comment))

The new code here should also be better.

TheNumbat · 2024-12-13T17:05:33Z

middle_end/flambda2/simplify/simplify_binary_primitive.ml

  let array_kind =
+    (* XXX mshinwell: should the array_load_kind be specialised too? *)


I'd guess no since unboxed load kinds are already known by the frontend?

TheNumbat · 2024-12-13T17:11:47Z

middle_end/flambda2/simplify/simplify_binary_primitive.ml

+            else
+              let index = Targetint_31_63.to_int imm in
+              let num_elt_kinds = List.length elt_kinds in
+              let elt_kind = List.nth elt_kinds (index mod num_elt_kinds) in


CR: packing

TheNumbat · 2024-12-13T17:13:56Z

middle_end/flambda2/simplify/simplify_binary_primitive.ml

+              let elt_kind = List.nth elt_kinds (index mod num_elt_kinds) in
+              if not (K.equal (K.With_subkind.kind elt_kind) result_kind)
+              then
+                (* CR mshinwell: I think this should try to reinterpret *)


TheNumbat · 2024-12-13T17:19:10Z

middle_end/flambda2/simplify/simplify_binary_primitive.ml

@@ -959,36 +951,38 @@ let simplify_array_load (array_kind : P.Array_kind.t)
      SPR.create named ~try_reify dacc
    in
    let[@inline] contents_unknown () =
-      return_given_type (T.unknown (P.result_kind' prim)) ~try_reify:false
+      return_given_type (T.unknown result_kind) ~try_reify:false
    in
    (* CR mshinwell/vlaviron: if immutable array accesses were consistently
       setting [mutability] to [Immutable], we could restrict the following code
       to immutable loads only and use [T.meet_is_immutable_array] instead. *)


Not directly related, but we can do this now

middle_end/flambda2/terms/static_const.ml

TheNumbat · 2024-12-13T17:26:55Z

middle_end/flambda2/terms/static_const.ml

@@ -164,47 +171,47 @@ let [@ocamlformat "disable"] print ppf t =
      Flambda_colours.static_part
      Flambda_colours.pop
      (Format.pp_print_list
-        ~pp_sep:(fun ppf () -> Format.pp_print_string ppf "@; ")
+        ~pp_sep:(fun ppf () -> Format.fprintf ppf "@; ")


No longer needs Format.

middle_end/flambda2/terms/static_const.ml

TheNumbat · 2024-12-13T17:40:32Z

middle_end/flambda2/to_cmm/to_cmm_shared.ml

+      naked_int64s
+    | Naked_number Naked_float -> naked_floats
+    | Naked_number Naked_vec128 -> naked_vec128_fields
+    (* The "fields" update kinds are used because we are writing into a 64-bit


CR: packing

TheNumbat · 2024-12-13T17:50:49Z

middle_end/flambda2/types/provers.ml

+    | [element_kind] -> (
+      match K.With_subkind.kind element_kind with
+      | Value -> Proved false
+      | Naked_number Naked_float -> Proved true


Agreed, but the frontend will rule out unboxed products of size 1 (#3317 (comment))

TheNumbat · 2024-12-13T18:00:42Z

middle_end/flambda2/types/reify.ml

            match
              try_to_reify_fields env ~var_allowed alloc_mode
                ~field_types_and_expected_kinds
            with
-            | Some fields -> Lift (Immutable_value_array { fields })
+            (* XXX mshinwell: We need to convince ourselves that there is no
+               problem with making different decisions based on immediacy


I assume this means static arrays of immediates will become non-scannable blocks? I can't think of any reason that'd be a problem, but may be worth considering stuff like this

mshinwell · 2024-12-20T19:31:16Z

Setting back to draft whilst work is in progress.

mshinwell added flambda2 Prerequisite for, or part of, flambda2 unboxed types labels Dec 11, 2024

mshinwell requested review from TheNumbat and lthls December 12, 2024 13:51

lthls reviewed Dec 13, 2024

View reviewed changes

TheNumbat reviewed Dec 13, 2024

View reviewed changes

mshinwell added 3 commits December 20, 2024 17:02

Static allocation / simplification for unboxed product arrays

1f42def

Move Array_kind into its own module

8a13b99

WIP

d8e9441

mshinwell marked this pull request as draft December 20, 2024 19:31

mshinwell force-pushed the flambda2-upa-static-alloc branch from 2f4cfcd to d8e9441 Compare December 20, 2024 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Static allocation / simplification for unboxed product arrays #3363

Static allocation / simplification for unboxed product arrays #3363

mshinwell commented Dec 11, 2024

lthls Dec 13, 2024

TheNumbat Dec 13, 2024

mshinwell Dec 20, 2024

TheNumbat Dec 13, 2024

TheNumbat Dec 13, 2024

TheNumbat Dec 13, 2024

TheNumbat Dec 13, 2024

TheNumbat Dec 13, 2024

TheNumbat Dec 13, 2024

TheNumbat Dec 13, 2024

TheNumbat Dec 13, 2024

mshinwell commented Dec 20, 2024

		let array_kind =
		(* XXX mshinwell: should the array_load_kind be specialised too? *)

Static allocation / simplification for unboxed product arrays #3363

Are you sure you want to change the base?

Static allocation / simplification for unboxed product arrays #3363

Conversation

mshinwell commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshinwell commented Dec 20, 2024