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

WIP: Poison constants #379

Open
wants to merge 6 commits into
base: flambda2.0-stable
Choose a base branch
from

Conversation

chambart
Copy link

This is a first attempt of adding poison constants to flambda to represent arguments present in a branch and not in an other. This can arise from unboxing. For instance an example where this matter is: (it has to be compiled with -flambda-expert-max-inlining-depth 2)

type ('a, 'b) t = A of int * 'a | B of 'b

let[@inline] f b y z g =
  let v =
    if b then
      A (42, g y)
    else
      B (g z)
  in
  match v with
  | A (_, a) -> a
  | B c -> c

let[@inline] g x = 12

let test b y z =
  f b y z g

The f function is analysed twice: once for definition, and a second time when inlining.
The first time the variant is correctly unboxed and all is well. And the branches of the switch ends in:

      k11: apply_cont k7 apply_result/214 42 #0)
      k9: apply_cont k7 0 apply_result/215 #1)

But the second time, the branches join the apply_result/214 and 0 and apply_result/215 and 42.
For the first round this was not a problem because the typing environment was 'hand built' by
unbox_continuation_param which didn't do that join. But the second round can't do that. Hence the
result can't be optimized when the g function is inlined

In that case the 0 is replaced by a Poison constant.

A Poison constant is a new kind of constants of type Poison that is somewhat
equivalent to bottom, but not really. It is ok to have a variable of type poison in the environment,
but using its value turns into bottom. This is done by expand_head.

That way the join of the poison and 12 here is 12 which allows to propagate things further.

It is not the case right now, but poison constants can also be used to track undefined variables during
Un_cps that would allow some parameter sharing.

@chambart
Copy link
Author

This is a first draft. This is similar to LLVM poison value. They have two kind of undefined variables Undef and Poison.
I'm still not sure that we don't want Undef instead, it might be less risky.

@chambart
Copy link
Author

By the way after testing I think I will change the definition of the constant. I thought it would be simpler this way, but now I think it will be simpler to have a case Poison in the Const_data rather than a field

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant