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

Fix duplication of code in Cmmgen #1370

Merged
merged 9 commits into from Oct 6, 2017

Conversation

Projects
None yet
4 participants
@lthls
Copy link
Contributor

lthls commented Sep 26, 2017

In some very specific cases, translation of switch expressions from clambda to cmm can duplicate the code contained in a branch of the original match.
I saw this happen while compiling middle_end/lift_let_to_initialize_symbol.ml, and from there built a smaller example for reproduction purposes :

type t = A|B|C|D
type s =
  | G of t
  | E of t
  | H of t
  | F of (unit list * t)
  | I of t

let r = ref 0

let set x = r := x

let f x =
  match x with
  | E B | F ([()], B) -> set 0
  | E x | F ([()], x) when Sys.opaque_identity true -> set 1
  | E _ -> set 2
  | F _ -> set 3
  | G _ | H _ | I _ -> set 4

Compiling this example with ocamlopt -dclambda -dcmm will show the set 2 instruction getting duplicated in cmm, even though it only occurs once in clambda.

Only one of the duplicated copies is actually reachable, so there is no soundness issue that I'm aware of, but the duplication stays there through all the remaining passes, so at least for code size it makes sense to remove it.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Sep 26, 2017

After looking at the code I have two questions:

  1. what is the relation between the third commit, 62aa049, and the problem you describe?

  2. You change 'a t_store into ('a, 'ctx) t_store: an exit action is now identified by both an index and a context, and two actions keys are considered equal if the contexts are the same (the indices are ignored in this case). How does ('a, 'ctx) t_store with the new definition differs from using ('ctx * 'a) t_store with the old definition and a well-chosen equality function?

@lthls

This comment has been minimized.

Copy link
Contributor Author

lthls commented Sep 26, 2017

The problem is triggered when switches generated during translation to lambda code contain a limited number of cases, and no fail action. Since the switch structure also contains information about the number of constructors in the type, the following passes tend to generate actions for those missing cases, and those additional actions are not shared properly.
The third commit fixes the translation from lambda to flambda in this case, since the flambda AST can actually represent them accurately.

About the t_store type change, I first tried using a ('ctx * 'a) t_store in Cmmgen, but since the type given to t_store has to match the type act given as the argument to Switch.Make, it meant a lot of wrapping and unwrapping of expressions in Cmmgen.SArgBlocks and the functions using it.
While the changes would have been confined to Cmmgen, the diff would have been bigger, and this solution looks cleaner to me.

| Cexit (i,[]) -> Some i
| _ -> None
in
match index, cont with

This comment has been minimized.

@gasche

gasche Sep 26, 2017

Member

I apologize for the extreme nitpicking but would you mind matching on cont, index? That's the order they are always in.

lthls added some commits Sep 26, 2017

@lthls

This comment has been minimized.

Copy link
Contributor Author

lthls commented Sep 26, 2017

Following suggestions from @chambart I've duplicated the StoreExp module to reflect that the context is different between the standard switches and string switches.

@damiendoligez damiendoligez added this to the 4.07-or-later milestone Sep 27, 2017

@gasche gasche closed this Sep 27, 2017

@gasche gasche reopened this Sep 27, 2017

@lthls lthls referenced this pull request Oct 5, 2017

Open

Cmm invariants #1400

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Oct 5, 2017

(cc @maranget : do you have an opinion on this proposed change? )

@maranget

This comment has been minimized.

Copy link
Contributor

maranget commented Oct 5, 2017

Hi,

The change fixes a real problem. It uses the existing Store component appropriatly and should be scheduled for integration. Well done.

May I ask for the pull request authors to comment on the following suggestion:

Impact on file others than switch.ml and cmmgen.ml can be significantly reduced by renaming your Store module into, say, CtxStore and then adding a new Store module that would be the application of CtxStore with type context being unit.

Then, there would be no need to change matching.ml, lambda.ml etc.

@lthls

This comment has been minimized.

Copy link
Contributor Author

lthls commented Oct 5, 2017

I've pushed a commit that splits the t_store type and Store functor into context-aware and context-unaware versions, which indeed allows to revert the changes to unrelated files.

It is a bit disappointing that the zyva function had to be duplicated in the interface, but it logically follows from the duplication of the t_store type, which arose from the necessity to have differently typed act_store functions.
Turning zyva's argument from Arg.act t_store to Arg.act shared array would be cleaner, but again implies patches at every call to zyva.

Anyway, I'm not attached to any particular solution, so I can revert back or go for a middle ground solution if you think it's better.
Also, the two flambda commits are related to the problem but not to the solution, so I can remove them or re-submit them as part of an independent pull request if you'd prefer this patch to focus on the cmm switch.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Oct 5, 2017

(Personally I find the two zyva functions and the increase in number of functors and signatures a bit complex, and I find the patch and result less readable than just adding extra () parameters where the type-checker asks to.)

@maranget

This comment has been minimized.

Copy link
Contributor

maranget commented Oct 5, 2017

Im an not sure at all that the store_t type have to be duplicated. My idea was rather
for the old functor to be the application of the new one to a unit context:

In switch.mli

  1. resurect the old Stored signature
module type Stored = sig
  type t
  type key
  val compare_key : key -> key -> int
  val make_key : t -> key option
end

(2) Define Stored with context as a signature "extension"

module type CtxStored = sig
  include Stored
  type context
  val make_key : context -> t -> key option
end

(3) then define the two functors for building sharing stores with and without context,
Notice the type of mk_store in the second functor signature.

module CtxStore(A:CtxStored) :
    sig
      val mk_store : unit -> (A.t, A.context) t_store
    end

module Store(A:Stored) :
    sig
      val mk_store : unit -> (A.t, unit) t_store
    end

...

In switch.ml, Store will basically be:

module Store(S:Stored) = struct
  module Me =
    CtxStore
      (struct
        include S
        type context = unit
        let make_key () = S.make_key
      end)
  let mk_store = Me.mk_store
end

....

Ok, that way we still have some strange () as first arguments in most act_store calls. However,
the similarily useless argument is no more present for make_key functions...
Or am I wrong somewhere?

@lthls

This comment has been minimized.

Copy link
Contributor Author

lthls commented Oct 5, 2017

It works, and removes part of the patches in lambda.ml, bytegen.ml and so on, but the parts that call act_store or act_store_shared would still need to be patched, so it would have come short of the goal of keeping unrelated files unchanged.

But as @gasche pointed out, this is not a goal that we absolutely need to reach. I'll update the pull request to revert the t_store duplication (but keep Store and CtxStore distinct), so it should get more or less to what you had in mind.

@maranget maranget merged commit 796f419 into ocaml:trunk Oct 6, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

he32 pushed a commit to he32/ocaml that referenced this pull request Oct 6, 2017

Fix duplication of code in Cmmgen (ocaml#1370)
* Fix duplicates in Cmmgen when handling switches with no default and not all cases

* Improve handling of incomplete Lambda switches in Flambda

* Add test (for reference) and changes

* Fix nitpick

* Cleanup: split the switch stores to reflect usage

* Improve compilation of incomplete switches with flambda

* Split switch stores into context-aware and -unaware versions

* Credit reviewers

* Go back to a single Switch.t_store type
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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.