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

Introduction of [Backend_var] #2056

Merged
merged 1 commit into from Sep 28, 2018

Conversation

Projects
None yet
2 participants
@mshinwell
Copy link
Contributor

commented Sep 21, 2018

This patch replaces uses of Ident in the backend by two new types. Backend_var is the direct replacement: currently this has a type equality to Ident, but this should be removed in the future. There is also a second type Backend_var.With_provenance which is used when the corresponding value is in binding position (for example the bound variable in a Clet expression). This second type holds some extra information about the variable.

This change serves two purposes: firstly, to enable the communication of information relating to bound variables (specifically about their module path, location and whether they are a renamed version of some original Ident) through to the points which need such information for debugging information emission; secondly, as a step on the road to providing better isolation between the various types used for variable-like entities throughout the compiler.

@mshinwell mshinwell added this to the 4.08 milestone Sep 21, 2018

@mshinwell mshinwell requested a review from chambart Sep 21, 2018

@chambart
Copy link
Contributor

left a comment

That's a step in the right direction.

This would indeed be nicer to separate backend_var from ident quickly.
What are the uses that would make it difficult ?

There are probably a few more modules that would benefit from the:

module V = Backend_var
module VP = Backend_var.With_provenance

It's probably better if it is a bit more uniformly used.

type t = {
var : backend_var;
provenance : Provenance.t option;
}

This comment has been minimized.

Copy link
@chambart

chambart Sep 21, 2018

Contributor

You could reduce a bit the memory usage by pushing the option out, given that this is always none right now:

type t =
  | Without_provenance of backend_var
  | With_provenance of {
    var : backend_var;
    provenance : Provenance.t option;
  }

I think that the final version shouldn't have provenance as an option, as this should probably be the default. For other cases, it might be worth considering adding something saying: 'introduced by that transformation applied on the function coming from that location'

This comment has been minimized.

Copy link
@mshinwell

mshinwell Sep 24, 2018

Author Contributor

I've made this change, although the provenance isn't optional in the With_provenance case. I agree that in future we might add something along the lines of what you suggest.


type environment =
{ vars : Reg.t array Ident.Map.t;
{ vars : (Reg.t array * Backend_var.Provenance.t option) V.Map.t;

This comment has been minimized.

Copy link
@chambart

chambart Sep 21, 2018

Contributor

Wouldn't this be better as a Backend_var.Provenance.Map.t rather than adding one inside ?

This comment has been minimized.

Copy link
@mshinwell

mshinwell Sep 24, 2018

Author Contributor

I don't think so -- for occurrences of variables not in binding position, you only have Backend_var.t, not the version with provenance.

loop let_bound_vars args
| _::_, _::_ ->
(* The [let] sequence has ceased to match the evaluation order
or we have encountered some complicated argument. In this case
we empty the stack to ensure that we do not end up moving an
outer [let] across a side effect. *)
outer [let] across a svare effect. *)

This comment has been minimized.

Copy link
@chambart

chambart Sep 21, 2018

Contributor

This might be a sed typo...

This comment has been minimized.

Copy link
@mshinwell

mshinwell Sep 24, 2018

Author Contributor

Fixed

@@ -394,20 +399,20 @@ let let_bound_vars_that_can_be_moved ident_info (clam : Clambda.ulambda) =
!can_move

(* Substitution of an expression for a let-moveable variable can cause the
surrounding expression to become fixed. To avoid confusion, do the
surrounding expression to become fixed. To avovar confusion, do the

This comment has been minimized.

Copy link
@chambart

chambart Sep 21, 2018

Contributor

Hum...

This comment has been minimized.

Copy link
@mshinwell

mshinwell Sep 24, 2018

Author Contributor

Fixed

@@ -19,6 +19,8 @@
open Cmm
open Parsecmmaux

module VP = Backend_var.With_provenance

This comment has been minimized.

Copy link
@chambart

chambart Sep 21, 2018

Contributor

Is that required ?

This comment has been minimized.

Copy link
@mshinwell

mshinwell Sep 24, 2018

Author Contributor

Apparently not.

{fun_name = $3; fun_args = $5; fun_body = $7;
{fun_name = $3;
fun_args = $5;
fun_body = $7;

This comment has been minimized.

Copy link
@chambart

chambart Sep 21, 2018

Contributor

This too.

This comment has been minimized.

Copy link
@mshinwell

mshinwell Sep 24, 2018

Author Contributor

Fixed.


(* Auxiliary for accessing globals. We change the name of the global
to the name of the corresponding asm symbol. This is done here
and no longer in Cmmgen so that approximations stored in .cmx files
contain the right names if the -for-pack option is active. *)

let getglobal dbg id =
Uprim(Pgetglobal (Ident.create_persistent (Compilenv.symbol_for_global id)),
Uprim(Pgetglobal (
V.create_persistent (Compilenv.symbol_for_global id)),

This comment has been minimized.

Copy link
@chambart

chambart Sep 21, 2018

Contributor

This probably didn't need reindentation

This comment has been minimized.

Copy link
@mshinwell

mshinwell Sep 24, 2018

Author Contributor

Fixed

@chambart
Copy link
Contributor

left a comment

Except the few typos, this looks good to me. This should essentially be a noop right now. Filling all those provenance will be tedious !

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

@chambart I'm afraid you'd better look at this again---it conflicted badly in places with the recent Ident.create_local changes. All of your comments should have been addressed. I don't have the time to disentangle Backend_var from Ident right now---it involves fixing Closure, involving the addition of an extra environment. However I would hope someone might be able to find the time too look into this; it would be useful work. (It may be better to wait until after the backend primitives issue is resolved, too, since they can use Backend_var.)

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

(@chambart Please squash+merge if you are happy with this, by the way.)

@mshinwell mshinwell force-pushed the mshinwell:backend_var branch from 28bcdc8 to e97d88d Sep 26, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

I've squashed this into a single changeset so it will be easier to manage other pull requests based on this one.

@mshinwell mshinwell force-pushed the mshinwell:backend_var branch from e97d88d to 4ce9180 Sep 26, 2018

@chambart

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2018

Looks good to me.

@chambart chambart merged commit 2b5f13c into ocaml:trunk Sep 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.