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

Improve location information in Lambda code #2294

Open
wants to merge 13 commits into
base: trunk
from

Conversation

@mshinwell
Copy link
Contributor

commented Mar 6, 2019

This patch enhances the Lambda language so that finer-grained location information can be propagated to the middle end. Detailed location information of this form is required in order to produce a good user experience in the debugger.

At the same time we aim to ensure that our generated Lambda code contains accurate location information. At present, there are some significant gaps in coverage, particularly in code generated by the pattern matching and switch compilers. This patch fixes that. It is possible (probable) that some more tweaking is required as this is tested on more examples, but this should be a good start. It's pretty nice being able to step through complicated matches in the debugger.

This patch touches Cmmgen and Strmatch a bit: this is because they use Switch. I also introduced a handful of new types in the pattern matching compilation code to help make it easier to understand.

@trefis is on the hook to review this one, with thanks.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Two further notes: this patch includes GPR#2284. Also, it adds two new flags (-dlambda-loc and -drawlambda-loc) to print Lambda code with location information attached.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

Shouldn't locations be added on all constructors (and then switch to the usual representation with a record containing the location and the "desc")? For instance, why is there no location on Lassign?

I suspect that having locations everywhere, even if not strictly needed for the debugger support, could benefit to e.g. Bucklescript (to produce sourcemaps).

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

They might not be needed in all cases---I would have to think about each one---but in principle, I agree with what you are saying. The patch is in its current form because I can't really afford to spend any more time on this right now---it has taken a very long time to get even this far---and this version seems to produce pretty good results.

@mshinwell mshinwell force-pushed the mshinwell:lambda_loc branch 2 times, most recently from 715a459 to 31ab859 Mar 8, 2019

@trefis
Copy link
Contributor

left a comment

I'm sympathetic to @alainfrisch's suggestion. There currently seems to be no obvious rule to decide which lambda constructors are given an extra location, or two, or none, etc.
Plus Lifthenelse is a mess.

Otherwise things look mostly reasonable, apart from some code in Matching.compile_match which, I believe, incorrectly handles locations in the presence of guarded clauses.

middle_end/closure_conversion.ml Outdated Show resolved Hide resolved
bytecomp/lambda.ml Outdated Show resolved Hide resolved
bytecomp/lambda.ml Outdated Show resolved Hide resolved
let rec make_sequence fn = function
[] -> lambda_unit
let rec make_sequence loc fn = function
[] -> lambda_unit loc

This comment has been minimized.

Copy link
@trefis

trefis Mar 18, 2019

Contributor

I was sad to see make_sequence take an extra argument, and even sadder when I saw what it was used for.
I wonder if using Location.none here would be acceptable. Any opinion?

This comment has been minimized.

Copy link
@mshinwell

mshinwell Mar 18, 2019

Author Contributor

I'm not exactly sure, but I'd rather leave it as-is, given that it's been tested fairly extensively. Why is the extra argument problematic?

This comment has been minimized.

Copy link
@trefis

trefis Mar 19, 2019

Contributor

Well, the API feels weird... I might be happier with

let rec make_sequence ~nil fn = function
  | [] -> nil
  | ... (* unchanged *)

This comment has been minimized.

Copy link
@mshinwell

mshinwell Mar 26, 2019

Author Contributor

I've looked at this again, and I think we can actually just use Location.none here. If the generated code ends up having a move at the end (to load the unit value), then Selectgen should propagate the debuginfo from the previous instruction onto the move, which should be fine.

lambda * (int * (Ident.t * value_kind) list) * lambda * Location.t
| Ltrywith of lambda * Ident.t * lambda * Location.t
| Lifthenelse of
lambda * Location.t * lambda * Location.t * lambda * Location.t

This comment has been minimized.

Copy link
@trefis

trefis Mar 18, 2019

Contributor

I think that one deserves to be recordified. I initially "parsed" it as

(lambda * Location.t) * (lambda * Location.t) * (lambda * Location.t)

but it looks like it's actually:

lambda * (Location.t * lambda) * (Location.t * lambda) * Location.t

This comment has been minimized.

Copy link
@mshinwell

mshinwell Mar 18, 2019

Author Contributor

I think I will turn this one into a record.

This comment has been minimized.

Copy link
@mshinwell

mshinwell Mar 26, 2019

Author Contributor

This will be resolved in the next version.

let {pm=this_match ; ctx=this_ctx } = divide ctx to_match in
let lambda,total = compile_match repr partial this_ctx this_match in
lambda, jumps_map up_ctx total
let (_loc, lam), total = compile_match loc repr partial this_ctx this_match in

This comment has been minimized.

Copy link
@trefis

trefis Mar 18, 2019

Contributor

Why is _loc ignored instead of being returned? The pattern doesn't match what one can see in do_compile_matching above.

This comment has been minimized.

Copy link
@mshinwell

mshinwell Mar 26, 2019

Author Contributor

This has been fixed.

end

let compile_matching repr handler_fun arg pat_act_list partial =
let pats_act_list_of_pat_act_list (pat_act_list : pat_act_list) =

This comment has been minimized.

Copy link
@trefis

trefis Mar 18, 2019

Contributor

No. I forbid it.

This comment has been minimized.

Copy link
@mshinwell

mshinwell Mar 18, 2019

Author Contributor

What exactly is the complaint here?

This comment has been minimized.

Copy link
@gasche

gasche Mar 18, 2019

Member

Well the function name pats_act_list_of_pat_act_list is not very pleasant to read on the caller side.

Maybe we could find better formulations for your types pats_act_list and pat_act_list.

In general I'm not convinced by foo_list, why not use the more flexible foo list instead? So we could find a name for "a single pattern and the action to take" and "many parallel patterns and the action to take". Suggestions:

  • simple_clause versus multi_clause ("clause" means "something with the shape foo -> bar)
  • pattern_action versus row_action
  • pattern_clause versus row_clause
  • clause1 versus clause (urgh)

Note that for the function itself you could just write row_clause_of_pattern_clause and then do List.map on it at the callsite.

This comment has been minimized.

Copy link
@mshinwell

mshinwell Mar 18, 2019

Author Contributor

Well, it's foo_list because I think the whole data structure should have a single name; it's just I didn't choose a good one. If you and/or @trefis agree on a name, I will gladly change it.

This comment has been minimized.

Copy link
@mshinwell

mshinwell Mar 18, 2019

Author Contributor

(To clarify, I think the naming should reflect what the thing is, not what structure is being used to represent it.)

This comment has been minimized.

Copy link
@gasche

gasche Mar 18, 2019

Member

Another thing that may work is type 'a case = ('a * lambda * Location.t) (why not 'a * lambda Location.loc?), so pattern case and pattern list case, with type 'a cases = 'a case list.

This comment has been minimized.

Copy link
@trefis

trefis Mar 19, 2019

Contributor

Well the function name pats_act_list_of_pat_act_list is not very pleasant to read on the caller side.

Or at the definition point.

I like the 'a case proposition.
In the end, I'd go with the following definitions:

type row = pattern list
type 'a case = 'a * lambda * Location.t

(* ... *)

let mk_row_case (pat, act, loc) : row case =  ([pat], act, loc)

And then just List.map mk_row_case (I'm not a fan of that name, but it's less painful than the previous one) on the few places that need it.

This comment has been minimized.

Copy link
@mshinwell

mshinwell Mar 26, 2019

Author Contributor

I've dealt with all of this.

bytecomp/matching.ml Outdated Show resolved Hide resolved
bytecomp/matching.ml Outdated Show resolved Hide resolved
asmcomp/strmatch.ml Outdated Show resolved Hide resolved

@mshinwell mshinwell force-pushed the mshinwell:lambda_loc branch from a6fda65 to 45b0311 Mar 18, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@trefis Thanks for the review; I've pushed changes to fix some of your comments. Could you please resolve the relevant conversations, and then I will attend to the remainder, which I think are more substantial? (I may not be able to come back to this GPR until next week.)

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

(I also rebased this PR, which involved resolving some conflicts.)

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

I only looked at the matching.ml code out of curiosity. I think that the code would have been nicer if each piece of code and its location where kept as the same value, instead of being passed around as two independent things. This is what Location.loc is for, and you can reduce the Location. noise by having

type 'a located = 'a Location.loc = { txt: 'a; loc : Location.t }

The synonym type user_code = lambda located might be clarifying in terms of distinguishing code coming from the user program and code generated by the pattern-matching compiler.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

We could do that, and generally I would be in favour of such things (except without the extra type alias). However in this case I'm not sure if it's worth it. Bear in mind the proposed change increases allocation.
In the context of the Lambda.t type definition, for the if-then-else case, a record is arguably better anyway; and there aren't actually many other cases where this happens.

I have less of an opinion about using this structure in the matching code itself, but again I suspect the trade-off in increased allocation may not be worth it. @trefis knows far more about this code than I do, and will have a more informed opinion.

As long as this GPR is not felt to be materially decreasing the quality of either Lambda.t (and I think after record-ifying Lifthenelse it does not) or the pattern matching code, it would be better to get this in as-is with a view to adding further improvements afterwards. Such improvements could then be traded off against other potential improvements to the matching code as a whole. There is a large amount of work blocked on this GPR which we really cannot afford to delay any further, and for which human time is already rather scarce.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

To reply to @gasche suggestion of using Location.loc (which I believe he made twice), I think Matching is not the right place for it. What you're suggesting is basically what Alain said: "why not have a location on every lambda node?", except Alain was looking at the big picture and you were only looking at the ugly painting.

I also think the whole thing would look nicer if that change was made, however Mark doesn't seem like he wants to do that work; which I guess is lucky for me, because I'm not sure I'd have the patience to review even more such tedious changes.

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

I'm feeling sorry for having jumped into this discussion, because now I'm in the position of the person that says things nobody wants to hear...

I totally get the idea of saying "I improved this thing a bit, I don't have time for the further improvements you suggest right now so let's stay at that". It's pragmatic and efficient. But I don't think that the part of this PR that I looked at would qualify for that classification. It is rather a case of making things worse in the process of some Important Feature, when we have the impression that a better way exists. Sure, we can leave the cleanup to some other PR but, just as no one wants to do it now, no one will want to do it later and, if we merge now, we will just stay with a codebase that is noticeably harder to evolve and maintain.

If this was a smaller patch, on the road for a less important Important Feature, I'm super-confident that I would be the annoying person to ask for a cleaner rewrite. It is not actually that much work, and keeping the compiler codebase nice and maintainable is arguably Even More Important than most Important Features we add these days. (And I think we should try harder to make things nice from the start when we touch the compiler codebase, instead of aiming for whatever works.)

Some ideas:

  • If someone was willing to try a nicer way than passing locations separately everywhere (what I don't like is the fact that the location is decoupled from the thing it is locating, except in the naming of variables), I would be happy to review that part.
  • Maybe there is a way to try a better approach on only some fraction of the changes, and post a partial patch (doesn't need to compile) to see how that looks like. This way, we could evaluate alternatives (eg. "having lambda located in bytecomp/matching" vs. "having locations at all Lambda nodes") without having to do full iterations. (We should have done things this way from the start.)
@gasche

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

A minor point:

I have less of an opinion about using this structure in the matching code itself, but again I suspect the trade-off in increased allocation may not be worth it.

I don't think we care at all about the cost of allocating one extra block per Lambda node, or (in the case of my more restricted proposal) one extra block per pattern clause and right-hand-side.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

I thought some more about the placement of Location.t values within Lambda.t (cf. a comment from @trefis, #2294 (review)). For the moment, I'm only concerned with what is required for the debugging work, rather than anything more general. I think, save for turning Lifthenelse into a record, the current situation should be fine when augmented with a comment (unless I have made a mistake and the following criteria are not satisfied). The comment can explain the following criteria for the positioning of locations:

  • There needs to be one at the start of every basic block.

  • There needs to be one on any construct that expands to an "operation" (for example, the address of a symbol being moved into a register, or a primitive).

  • The Location.t values in the rightmost arguments of the constructors always correspond to the start of the construct itself, which is consistent with the existing ones in such positions.

Adding a record indirection for all nodes therefore seems overkill at the moment. I think that is also likely to be significantly to the detriment of readability, although depending on the rationale, that might be an acceptable trade-off in the future.

I must emphasise that this patch has already received a substantial amount of work (about two solid weeks)---it was far from trivial to thread the necessary location information through the pattern matching compiler. I found it extremely difficult to understand some of that, admittedly very clever, code due to extensive use of partial application and lack of explicit types. If we're worried about the maintainability of this area of code, then I'm afraid the place to turn attention to is the existing code, rather than being over-concerned about the relatively minor additions in this patch. I do not believe those additions will materially affect maintainability, at least once the Lambda.t type has been clarified.

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

For the record, I don't know if using a record wrapper in Lambda.t is the best approach to make code readable. What is immediately clear in the present patch is that you are passing term fragments and locations (or debug information) for these fragments separately, while it would be nicer if they were passed together as a single object, as it would make clear/obvious what locates what.

One aspect of this discussion I don't really like is the use of the argument "I already spent two weeks on this". You have ambitious goals regarding gdb which require a ton of work, and this is laudable. Writing good code is more work than writing meh code, which means that you have a ton more work, and we should budget for that from the start! Having a ton of work to do does not give anyone license to write bad code, and having discussions on how good the code is, and possibly asking for changes (to, yes, a lot of code), should be on the table for any pull request.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

Let's place a moratorium on any further comments until this is revised.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

By popular demand, I've pushed a new version of this, which should be much more satisfactory. There were so many additional changes that I've just squashed everything down to a single commit; the best starting point for review is the base revision.

I sat down with @lpw25 and we examined the Lambda.lambda type in detail. We have settled upon a particular rationale as to the placement of locations, which is documented in lambda.mli. I have also reflected this in the Cmm.expression type. There is now a particular notion of "block" which is used not only in the Lambda.lambda type (and analogously in Cmm.expression) but also as the type of "actions" during pattern matching compilation.

Whilst making these changes I'm afraid I felt bound to improve some of the formatting of various functions in matching.ml at the same time, together with improving some naming (and labelling various parameters), so as to make it easier for myself and others to understand what is going on. The diff doesn't look too bad with patdiff but the occasional function may be easier to review in its final state (rather than reading the diff). There will be some more review load imposed by this, but I am sure that it is worth it, with thanks in advance for reviewers' perseverance.

I believe I have addressed all of @trefis 's other comments, as above.

@gasche
Copy link
Member

left a comment

I haven't looked at all of parmatch.ml yet, but this looks much nicer. Thanks!

| c -> Cop(Cload (Double_u, Immutable), [c], dbg)

and unbox_float_block block =
let dbg = block.block_dbg in
Cmm.block dbg (unbox_float dbg block.expr)

This comment has been minimized.

Copy link
@gasche

gasche Mar 26, 2019

Member

(I don't know if this was already in the previous patch but this two-layer recursion scheme is actually quite elegant. Well done!)

This comment has been minimized.

Copy link
@mshinwell

mshinwell Mar 26, 2019

Author Contributor

This is new.

| Lassign of Ident.t * lambda
| Lsend of meth_kind * lambda * lambda * lambda list * Location.t
| Levent of lambda * lambda_event
| Lifused of Ident.t * lambda

and block = private {

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Mar 26, 2019

Contributor

When the "block" is a lambda node with a toplevel Location.t annotation, do the two locations necessarily match?

Why not go the full way and attach the location in a generic way to all lambda nodes (renaming block to lambda and lambda to lambda_desc)?

This comment has been minimized.

Copy link
@mshinwell

mshinwell Mar 26, 2019

Author Contributor

There's no constraint with regard to the two locations you identify. It may be preferable for these locations in some cases to be distinct -- for example if one arm of an Lifthenelse was a function application surrounded by begin and end, then it would be reasonable for the location on the block (in the Lifthenelse arm) to be that of the begin but the location on the application (inside the block) to be on the f x or whatever it is.

There's no need at present to attach the information to all nodes---at least for the main purpose of this pull request. I did look at this, and concluded that it seemed unnecessarily disruptive.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

It may be preferable for these locations in some cases to be distinct -- for example if one arm of an Lifthenelse was a function application surrounded by begin and end, then it would be reasonable for the location on the block (in the Lifthenelse arm) to be that of the begin but the location on the application (inside the block) to be on the f x or whatever it is.

I see, but this is not what happens in this PR, right? Anyway, we won't be able to do something more fine-grained than what the Typedtree keeps, which is currently one location for each node.

The natural approach, in my opinion, would be to keep the same scheme as for Parsetree or Typedtree, i.e. attach the location information at each node, in a uniform way. If only for the sake of design uniformity across the compile code base. The introduction of "blocks" goes a long way towards this approach, so I don't understand whether you prefer not to do that because (1) this would be more (invasive) work and you really want to move on -- which I can fully understand, or (2) there are intrinsic benefits of not doing it like that (in which case, I wonder if the same arguments would apply to Parsetree and/or Typedtree).

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

@alainfrisch I'm not actually sure I know the answer to the first question in all cases -- but I don't think it really matters. The location information is in any case very likely to require tweaking, quite possibly in ways we have not yet foreseen, once we get this whole package of work released.

I explained in the previous comment that I see no technical merit for having anything beyond the existing "blocks" approach at this stage. I don't think we should be cluttering up intermediate languages with unnecessary information, both because it can materially impact upon code clarity at sites where terms are constructed and deconstructed, and because it is wasteful at runtime.

I do also really want to move on, so we can get the benefits of this work to users. To this end, I would very much appreciate feedback on the actual logic within this patch, in particular the pattern matching compilation parts. The idea is that the locations put on terms by the match compiler should reflect the progress of a pattern matching at a fine level to the fullest extent possible.

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

(2) there are intrinsic benefits of not doing it like that

In the middle-end languages (lambda, clambda, flambda, cmm) it doesn't make sense to talk about the locations of many of the constructs. Things like lets and variable uses are artefacts of the IRs themselves not constructs in the original source, and not things that produce instructions in the output. The things that locations do make sense for are essentially the operations (e.g. primitives and function applications) which should always correspond to a corresponding expression in the source and which directly produce assembly instructions, along with parts of the control structure (e.g. the branches) which also appear in the source and can be "blamed" for instructions in the output. The current design of this PR attempts to put locations on all of those places.

x1::jumps_union rem1 env2
else
x2::jumps_union env1 rem2
type jumps_union = (int * ctx list) list

This comment has been minimized.

Copy link
@gasche

gasche Mar 30, 2019

Member

This type declaration is wrong (the function does a _union but the datatype is not a union); it should be jumps, which is already defined above, and you could use this to annotate env1 if you wanted to.

This comment has been minimized.

Copy link
@mshinwell

mshinwell Apr 9, 2019

Author Contributor

I've fixed this and actually made a new Jumps module which cleans this up rather more satisfactorily. It turns out that the type can just be made abstract.

gasche added a commit to gasche/ocaml that referenced this pull request Apr 15, 2019

lambda/switch.ml: pass location information to the make_* combinator …
…parameters

The Switch module is responsible for compiling switches on intervals
of integer values into efficient trees of tests. It is structured as
a functor parametrized on test-generation primitives, to be used by
both the pattern-matching compiler at the Lambda-code level in
lambda/matching.ml and some lower-level Cmm-level operations in
asmcomp/cmmgen.ml

We are working on passing precise location information from the
frontend to the backend. In particular, Switch should track the
location of the input values it is matching over, and assign the
corresponding locations to the tests it generates.

The present commit is a first step in this direction: it adds an
abstract type of locations to the functor argument (to be instantiated
as Location.t for lambda-level switches, Debuginfo.t for cmm-level
swtiches), and updates the users of the functor to this new API. The
compilation logic in Switch itself is unchanged, so for now the
locations being passed are all dummies.

(The `no_loc` argument is hopefully temporary; it should be the case
that, after the Switch code is updated to track locations correctly,
we never need to make up locations during the compilation, so that
`no_loc` can be dropped from the functor interface.)

(This code was actually assembled together by Gabriel Scherer from
Mark's ocaml#2294)

gasche added a commit to gasche/ocaml that referenced this pull request Apr 15, 2019

lambda/switch: take as extra argument a location for each switch case
This extra map passed to the entry points will let the Switch module
generate code with more precise location information. This commit does
not update the code-generation machinery, only the interface; dummy
locations are currently provided by the callers, and separate work
will provide more precise locations there (see ocaml#2294).
@gasche

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

I had been thinking about doing a bit of hacking for this PR since last week; don't pry that away from me!

I did it this afternoon anyway, and it's very simple (using approach (3), but I believe that other approaches would be equally simple to implement). The whole branch is there, but it mostly consists of buildup noise (I worked on top of trunk, reusing a bit of Mark's code and changing the users to provide dummy location maps; this would have to be rebased on top of this PR and adapted), with the (surprisingly small) real work going in 2ebb7fa.

@gasche

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

The reason why it's simple/small: @maranget had done a very nice job implementing this, with a clean design where all the algorithmic cleverness goes into building an abstract description of the test trees (where to split, etc.), and a single handler at the end that recursively traverses this abstraction description and generates the corresponding code. Only the handler (c_test) needs to be modified to pass location information.

fprintf ppf "@[<hv 1>case int %i:%a@ %a@]"
n
print_loc act.block_loc
lam act.expr)

This comment has been minimized.

Copy link
@gasche

gasche Apr 15, 2019

Member

you should have a block or print_block function that does the right thing so that this can be rewritten as

fprintf ppf "[@<hv 1>case int %i:%a@]" n block act
@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Can people involved in this PR arrange to make progress? It is blocking related work, but also unrelated one such as #2165. So please don't let this PR stall.

@gasche

This comment has been minimized.

Copy link
Member

commented May 9, 2019

I don't think this PR should be considered blocking for the unrelated #2165. It would be blocking if the other PR depended on a change in this PR (this is not the case), and it would be "nice to wait" to avoid a rebase if the present PR was to be merged imminently (I don't think this is the case either). Let's set #2165 free to live its own exciting PR life.

As to the status of the current PR: personally I think that having more location information in Lambda code is a desirable feature for a variety of use-cases (not in the sense that I can personally name any one use-case that I deeply care about right now, but in the sense that I am convinced reasonable use-cases exist), and I would be willing to finish the review work for this PR (getting Thomas to review my own changes to switch.ml; I also have one-fourth to one-third of the code still to review, which is also likely to require cleanup changes after review) and to get it merged.

On the other hand, clearly @xavierleroy is not happy with the idea of adding more location information in Lambda code, independently of how invasive/ugly the change is (the first version was fairly ugly but I think it can be made a very reasonable, readability-preserving change). I think that it could be possible for me to sneak the commit inside the Treasure Cave without awakening the dragon -- or maybe to persuade the dragon to look elsewhere -- but I have not tried yet. I don't think waiting a few extra weeks would do us any harm.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@gasche Let's please have some discussion between the various people involved in changes (or potential changes) to Cmmgen before proceeding -- we could do this on email. The continual effort to rebase these patches on all sides is significant, and we should try to co-ordinate.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

The dragon metaphor is no quite right. The duty of the dragon is to make sure that nothing is taken off the treasure cave. My duty is to make sure that only treasure, and not litter, is stored in the treasure cave. You don't change a part of the compiler as sensitive as the pattern-matching compiler just on a hunch that more location information must be good and may be useful in an indefinite future.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Independently of arguments about dragons, one thing to note is that we could do large-scale testing on pull requests such as this to demonstrate that the generated code does not change.

@lpw25

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

just on a hunch that more location information must be good

My understanding is that this PR does fix bugs in the locations that OCaml currently outputs for things (maybe in combination with another patch to propagate the data from cmm to the asm I'm not sure). Fixing these bugs seems worth at least the relevant parts of this code -- which I think is basically the non-Switch parts.

@trefis

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

getting Thomas to review my own changes to switch.ml; I also have one-fourth to one-third of the code still to review, which is also likely to require cleanup changes after review

I intend to keep my word on that part, however considering the recent discussions about this PR, I think that the changes to Switch should at least be split out of this feature (if they are to be included at all).
Indeed, from what I understood:

  • that part of the changes is the most controversial
  • we do not have any example where they would be useful
  • there are a few different choices (3 IIRC) we could make/should explore w.r.t. what the locations should be

If that part was to be taken out¹, would Heimdallr agree to it being merged?


1: And perhaps some examples were given of situations were this patch does indeed improve the reported locations?

@mshinwell mshinwell force-pushed the mshinwell:lambda_loc branch from 63cb475 to 6ea9aeb May 16, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

I've removed the Switch parts; we can come back to those later. I've also rebased this onto current trunk.

Almost all the changes to Cmm have also been removed from this patch. I'm going to present some of these as a separate PR as I think they have a fair amount of value in themselves (in summary, using the same "block" notion as this patch uses for Lambda simplifies the Cmm language types).

I would like to argue again that fine-grained location information is of real value for applications other than single-stepping in debuggers. (In fact, the position of single-step points should be marked independently from the location information, but only now are debugging information formats evolving to the point of being able to support that.) This morning, I heard another potential application, which was recovery of accurate source locations as to where particular optimisations had applied. This was in the context of assessing the applicability of existing, mainly Cmm-based, optimisations and deciding whether some new optimisation was worthwhile.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

This patch also helps to fix existing problems with the compiler's line number information output (along with a small subsequent patch to Selectgen). Here is a way of visualising an example reported by a developer at Jane Street:

  • Go to godbolt.org
  • Select language OCaml from the dropdown menu
  • Paste the following code into the left-hand pane:
let [@inline never] helper m x = Some (m + x)

let f m x =
  match helper m x with
  | None -> x
  | Some x -> 1 + x
;;
  • On the right-hand side the coloured blocks in the assembly code correspond to the coloured lines in the source pane. It can be seen that the location information emitted for both branches of this match are the same!
@gasche

This comment has been minimized.

Copy link
Member

commented May 16, 2019

One use-case I have in mind for location/debug information in pattern-matching (including the Switch part) is profiling tools that let users study mispredicted branches in the binary, and use DWARF information to let users go back to the source code for each branch.

I have seen real-world cases of users wondering about misprediction in pattern-matching-generated code (namely, a case where using a polymorphic variant was faster than a normal variant due to branch predictors fitting the dynamic distribution of input data in the tree-of-tests generated for the (non-consecutive) polymorphic variant tags, beating the jump-table implementation used for (consecutive) variant tags).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.