Skip to content

Allow Simple dominators in Flow Analysis (instead of just variables) #2782

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Gbury
Copy link
Contributor

@Gbury Gbury commented Jul 15, 2024

This PR adds the ability for the data flow analysis to discover any Simple as a dominator, whereas previously only variables could be detected as dominators. In particular, that means that we can now detect (and add aliases) when a variable is aliased/dominated by a symbol or a constant.

Whenever we add such an alias to a constant, or a symbol (with a known type), we trigger a resimplification, since that opens up new opportunities for Simplify . This is in particular necessary for the example below to work.

This (finally) solves #2123 , so that in the following example:

[@@@ocaml.flambda_o3]

type 'a t = {
  mutable x : unit;
  get : int -> local_ 'a
}

let f j =
  let local_ t = { x = (); get = (fun x -> x) } in
  for i = 1 to j do
    Sys.opaque_identity ((t.get [@inlined hint]) i : int)
  done

The t.get function is indeed inlined inside of the loop. Unfortunately, this required three rounds of simplification (the first one does the mutable unboxing, the second one adds the alias to the symbol for the lifted function, and finally the third one can do the inlining). In the future, we could do it with only two passes if we ran a second alias analysis after the mutable unboxing (but that will require quite a bit of work).

It would be good if we could try and benchmark compilation times with this PR to checks whether the new code triggers too many resimplifications.

@Gbury Gbury added the flambda2 Prerequisite for, or part of, flambda2 label Jul 15, 2024
@Gbury Gbury requested review from chambart and Ekdohibs July 15, 2024 15:14
@mshinwell mshinwell changed the title Allow Simple dominators in Flow Analaysis (instead of just variables) Allow Simple dominators in Flow Analysis (instead of just variables) Jul 15, 2024
@mshinwell mshinwell linked an issue Jul 15, 2024 that may be closed by this pull request
@Gbury Gbury force-pushed the data_flow_symbols branch from b81a32f to 68c8ea1 Compare July 15, 2024 16:00
@Gbury Gbury force-pushed the data_flow_symbols branch 2 times, most recently from 560947e to 17d2c2f Compare October 7, 2024 13:06
Copy link
Contributor

@chambart chambart left a comment

Choose a reason for hiding this comment

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

LGTM, only some wondering about the resimplification criteria

@Ekdohibs
Copy link
Contributor

I'm wondering how safe this is w.r.t. mutable unboxing: if a mutable block ends up in a symbol, it must be marked as potentially escaping since the block could be modified by directly accessing its symbol.

@Gbury
Copy link
Contributor Author

Gbury commented May 28, 2025

I'm wondering how safe this is w.r.t. mutable unboxing: if a mutable block ends up in a symbol, it must be marked as potentially escaping since the block could be modified by directly accessing its symbol.

I think we're safe because of the following reasons:

  • mutable unboxing only triggers if the dominator is the mutable allocation
  • symbols are lifted at top-level, so mutable unboxing involving a symbol can only trigger at top-level
  • at top-level, we can effectively see all uses of the symbol, including if it's stored in the module block, so we should correctly compute whether it escapes or not

Note that if/when we try to not move symbol definitions at top-level (and instead tweak the let-binding in place to indicate it's now a symbol), this reasoning will not hold anymore.

@Gbury Gbury force-pushed the data_flow_symbols branch from 8174810 to b3db9b9 Compare May 28, 2025 09:18
@Gbury Gbury requested a review from chambart May 28, 2025 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flambda2 Prerequisite for, or part of, flambda2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure of mutable unboxing in conjunction with a for-loop
3 participants