Flat map statements into blocks, and use in Aslp by katrinafyi · Pull Request #217 · agle/bincaml

katrinafyi · 2026-07-10T07:39:19Z

This adds a function for mapping a single statement into one or more blocks (or statements), while fixing up all the control flow.

type ('v, 'e) mapped_stmt =
  [ `Stmts of (Var.t, Var.t, BasilExpr.t) Stmt.t list
  | `Blocks of (Var.t, BasilExpr.t) Block.t list
  | `Graph of ID.t * ID.t * ('v, 'e) t ]

let general_flat_map_stmts
    ~(f : proc:_ t -> _ Stmt.t -> _ mapped_stmt)
    base_bid
    proc

The function f should return multiple blocks by returning a (first_id, last_id, proc') where first and last are new blocks which f inserts into the updated proc'. Alternatively, it may also return a stmt_list. In this case, it is assumed that the procedure doesn't change.

The implementation is a bit big but hopefully it's normal. As well as being useful for Aslp, I'm sure this will be useful in other cases too. For example, maybe for inlining procedure calls.

This PR also changes the Aslp frontend to make use of the new flat_map_stmts. This leads to much simpler code. The Aslp transform no longer needs to split the statement list into before+intrin+after, since we can just handle one statement at a time. Impressively, there are very minimal changes to the expect test output.

It also fixes the quadratic runtime of the current code in main. There's also a expect test to verify this. If you want, this can be deleted since timing-based tests can easily become flaky.

I have also left some TODO comments in the diff and want some advice on those. I'm also not sure about the name of the function, there's already a few functions with similar names..

plan: - writing isolate function is too hard? - flat_map : stmt -> (proc * id * id | block | stmt)

always include them

because we would need to compute the one *after* the group in order to know when to end the group. therefore, if we're in a bare statement group, we could still compute a block-pair ID before returning the bare statement group. i guess we would have to make it a Seq in the group lists too. but that seems hard to avoid re-computing. maybe just use persistent??

Seq cannot have foldmap

This reverts commit 58145f0.

…ent?" This reverts commit 778eef6.

agle · 2026-07-13T01:32:25Z

The function f should return multiple blocks by returning a Left (first_id, last_id, proc') where first and last are new blocks which f inserts into the updated proc'.

Does this strictly insert two blocks from proc' into proc? Or does it take all blocks between these assuming a diamond? What happens if the caller makes unrelated changes to the procedure in ~f?

Var.t, Var.t, Program.e) Stmt.t ->
    ( ID.t * ID.t * ('a, 'b) Procedure.t,
      (Var.t, Var.t, Program.e) Stmt.t list )
    Either.t

This should be a defined type or polymorphic variant with names for the cases.

What is the base_bid, last ID.t argument?

katrinafyi · 2026-07-13T07:16:27Z

I think I've done the suggestions now :) The functions are moved around, renamed, and better documented.

agle · 2026-07-14T04:52:54Z

+    (** Zero or more straight-line statements. *)
+  | `Blocks of (Var.t, BasilExpr.t) Block.t list
+    (** Zero or more straight-line blocks. *)
+  | `Graph of ID.t * ID.t * ('v, 'e) t


If declaring the type it may as well be a regular variant as it provides better type inference and errors.

Variants are kinda annoying to use because you have to qualify them with the module. Also, a variant like Procedure.Blocks is rather confusing I think, so it would need a different name anyway. And it's really just a special type used in this one function that is only declared separately so I can ocamldoc it.

Unless you feel strongly, I will leave it.

agle · 2026-07-14T05:00:15Z

+(** [general_flat_map_stmts ~f bid proc] maps each statement in the given block
+    ID through [f]. For each statement, [f] may return either zero or more
+    "bare" statements, {i or} a first/last pair of new block-level control-flow.
+    See {!type-mapped_stmt} for details about [f]'s return type.


Or list straight-line list of blocks.

agle · 2026-07-14T05:10:35Z

+
+    Additionally, redirects the original block's incoming/outgoing edges to the
+    first / last block of the mapped output. *)
+let general_flat_map_stmts ~(f : proc:_ t -> _ Stmt.t -> _ mapped_stmt) base_bid


redirects the original block's incoming/outgoing edges to the
first / last block of the mapped output

This seems subtle—trivial for Stmts, obvious for Blocks, but the Graph case could do odd things like remove the successors of the original block, or add additional successors. I think it should be made clear that the Graph case need not produce an additional subgraph that is connected to the original? Its just a new subgraph detached in the procedure (but in the same forest) that gets linked up by this function?

One or both of [first]/[last] may be the same as the original block.

Not sure if this case is more confusing to allow; might want to force to return blocks or stmts in that case.

Could a version of the API generate fresh begin + end blocks of the diamond and pass them to a closure returned by the Graph case to inject the diamond between? That way you would be appending / prepending to a block and have it be automatically bookended; it could check the succ/pred sets of the end points aren't modified. Maybe it could simplify the other special cases where it can concat to a prev block as well.

Sorry I say all this because its not obvious how I would use this immediately; e.g. what assumption do you make of the state of the graph within f when processing an arbitrary stmt of the program in order to modify the graph? And what do you have to guarantee about the graph you return?

Maybe the subgraph refactor would be the easiest way to clean that up though.

That's cool but I'm probably not going to do that (have the flat_map implementation provide bookend blocks). I don't think it's a qualitative improvement over the current approach if we're still relying on Procedure to store the block-level control-flow.

This seems subtle ...

Sorry I say all this because its not obvious how I would use this immediately

f's purpose is to map one statement. So, f should make no assumptions at all on the state of the graph.

Are your questions adequately answered by the doc comment on `Graph? If not, how can it be improved? I think it answers all of your questions, just not in the same graph theory words.

agle · 2026-07-14T05:12:23Z

+let general_flat_map_stmts ~(f : proc:_ t -> _ Stmt.t -> _ mapped_stmt) base_bid
+    proc =
+  let existing_bids =
+    lazy (iter_blocks proc |> Iter.map fst |> Iter.to_set (module IDSet))


Why not IDSet.of_iter?

I didn't know it 🤯

agle · 2026-07-14T05:17:50Z

+                       ~stmts:(CCVector.to_list stmts) ())
+                   proc bs
+               in
+               ((proc, None), List.map (fun x -> Either.Left (x, x)) bids)


What are Left and Right representing in this result list?

moved into a local function and added a comment above that local function

agle · 2026-07-14T05:23:40Z

+    if not ID.(equal first base_bid) then
+      add_goto ~from:base_bid ~targets:[ first ]
+    else Fun.id
+  in


Might help clarify the API if you also require that f : Graph case does not modify the control flow of the original block.

agle · 2026-07-14T05:27:54Z

+       (snd %> Procedure.iter_blocks
+       %> Iter.flat_map (fun (_, b) ->
+           Iter.append (Block.read_vars_iter b) (Block.assigned_vars_iter b)))
+  %> Iter.filter Var.is_global


It should be a requirement that this should always be less than Program.global_vars. It should be covered by checks.ml.

Program.global_vars filters the decls list, but here we need to get referenced variables which may or may not have been declared.

agle · 2026-07-14T05:34:37Z


 (** Modify stmt list by creating a mutable copy of the underlying vector *)
-let fmap_stmts_copy (f : (('a, 'a, 'b) Stmt.t, 'c) Vector.t -> unit) b =
+let fmap_stmts_copy (f : (('a, 'a, 'b) Stmt.t, 'mut) Vector.t -> unit) b =


Should 'mut be the specifc mutable type defined in Vector rather than generic?

no. in CCVector, rw and ro are two totally distinct types.

I know that, I thought it was being passed a mutable vector specifically, but Vector.copy is shallow and just returns the same vector if its immutable.

IDSet.of_iter missing-record-field-pattern Or list straight-line list of blocks.

katrinafyi added 18 commits July 9, 2026 10:10

stmts_of_aslp_block

0b4751e

starting flat_map_block but can't think

3e80e6a

isolate_stmts_of_block

66166bc

plan: - writing isolate function is too hard? - flat_map : stmt -> (proc * id * id | block | stmt)

isolate_stmts_of_block probably works now

a065b1c

touch

d6c9ce9

stash

2ae82ed

it compiles

aee7b60

touch and fix base_bid stmts not being deleted

a908d88

nonempty list

f9892e1

support stmt list as flat_map function output

045d47b

comments

ce192a5

add linear test

224968c

delete unused test file

4d908c9

x

3c276e1

use Aslp_util_internal.flat_map_stmts 🤩🤩

7e789a3

don't need error_attrib_key or include_failed anymore. we can just

4507b64

always include them

remove unused functions

9467549

remove unused function

6d6cbcb

katrinafyi requested a review from agle as a code owner July 10, 2026 07:39

katrinafyi added 8 commits July 11, 2026 17:54

less characters

a9aef85

jfidosajfoidsajfidosa it not good

58145f0

Seq cannot have foldmap

Revert "jfidosajfoidsajfidosa it not good"

d3af79c

This reverts commit 58145f0.

Revert "blah. does it even work with the eager list in the first elem…

876cc14

…ent?" This reverts commit 778eef6.

headings

729de80

touch. clear_stmts

5ae181d

x

7ba1401

katrinafyi added 2 commits July 13, 2026 14:42

move to extras.ml

00a9f7a

move referenced_vars_of_prog

527643d

katrinafyi added 5 commits July 13, 2026 14:48

move flat_map_stmts

9c6ccf2

rename to general_flat_map_stmts until we delete the other ones

71d0674

move proc argument to end for piping

63f0aa0

add and document mapped_stmt type

329827e

assert that `Block IDs are fresh

62722ba

katrinafyi and others added 4 commits July 13, 2026 17:23

Update common.ml

27bb038

add `Blocks in case the user wants to return straight line blocks

dd211e9

Update common.ml

1919e34

fmt

f1a00f1

agle approved these changes Jul 14, 2026

View reviewed changes

katrinafyi and others added 2 commits July 14, 2026 17:25

working...

7fd0929

IDSet.of_iter missing-record-field-pattern Or list straight-line list of blocks.

fix docs

c59fd93

agle enabled auto-merge (squash) July 15, 2026 02:55

agle merged commit 52212f3 into main Jul 15, 2026
9 checks passed

agle deleted the fjdoisafda branch July 15, 2026 02:58

Conversation

katrinafyi commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agle commented Jul 13, 2026

Uh oh!

katrinafyi commented Jul 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

katrinafyi Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agle Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

katrinafyi commented Jul 10, 2026 •

edited

Loading

katrinafyi Jul 14, 2026 •

edited

Loading

agle Jul 15, 2026 •

edited

Loading