Doubly-linked lists for CFG layouts

[xclerc]

This pull request does two things:

• extract the doubly-linked list representation of
  instruction lists so that we can have doubly-linked
  list of any type;
• use such doubly-linked list for CFG layouts.

[gretay-js]

• The use of Obj.magic 0 is slightly better than a dummy instruction. Just to confirm , the use of '0' here will result in a tagged (immediate) value stored in the value field, right?
• is_dummy_node will be fine with marshal/unmarshal because the self-loop is a "local" check, but we will end up with more than one "dummy" node in this scenario. I think it is possible in this scenario after a few add/remove operations to have an empty ist in which first and last are not the same or first.prev and last.prev not pointinng to the same node. I think it's fine and we don't rely on it anywhere at the moment. Maybe worth a comment?
• The generic new doubly link list module doesn't need to live inside Cfg. It would be nice to split it out in a follow-up PR. cfg.ml is getting too long.

[Gbury]

I would really prefer if we could avoid using Obj. How about we first start with an Obj-free implementation of doubly linked lists (that could be shared with #1124 and the lru cache used for cmx files, as suggested by @Ekdohibs ), and then if needed for performance, we can consider optimisations, of the Obj or (preferably) non-Obj variety.

[xclerc]

Which call(s) to Obj.magic should be removed?
(There is https://github.com/ocaml-flambda/flambda-backend/pull/1124/files#diff-11eebe500261a6e20c905f76be4f78ba30f193ca9b0b1203d7056fc18880b073R46 and https://github.com/ocaml-flambda/flambda-backend/pull/1124/files#diff-11eebe500261a6e20c905f76be4f78ba30f193ca9b0b1203d7056fc18880b073R48.)

[Gbury]

Both ? My point is that using Obj should really be a very last resort, once a proper implementation is in place, and the need for it has become necessary for performance reasons (and there is no other way to restructure the code to get the required performance improvements). In short, a first implementation (as is the case here for the doubly linked list if I'm not mistaken) should not use Obj right out of the gate.

[lpw25]

I don't really agree that you should have to write a version without Obj first. This data structure is being added specifically for performance reasons. If you need Obj to write the fast version then that's fine. However, I would suggest using Obj.t rather than casting between ordinary OCaml types. Currently the code lies to the compiler about what type something has. Whereas if you write it along these lines you can avoid lying:

module Node : sig

  type 'a t
  
  module Neighbour : sig

    type 'a node := 'a t

    type 'a t
    
    val is_dummy : 'a t -> bool
    
    val get : 'a t -> 'a node

  end
  
  val prev : 'a t -> 'a Neighbour.t
  
  val next : 'a t -> 'a Neighbour.t
  
  val value : 'a t -> 'a

end = struct

  type 'a t =
      { value : 'a;
        mutable prev : Obj.t t;
        mutable next : Obj.t t
      }
      
  let rec dummy_node =
      { value = Obj.repr 0; prev = dummy_node; next = dummy_node }
      
  module Neighbour = struct

    let is_dummy t = t == t.next

    let get t =
      if is_dummy t then raise Not_found
      else (Obj.magic (t : Obj.t t) : 'a t)
     
    type nonrec 'a t = Obj.t t
    
  end
  
  let prev t = t.prev
  let next t = t.next
  let value t = t.value

end

[Gbury]

I don't see why we need the Obj in this case, even for performance. If I'm not mistaken, the uses of Obj are mainly due to the fact that the doubly linked list is polymorphic and that the code uses a single dummy_node for all instances of doubly linked lists. Why not use a functor (inlined with annotations if you're worried about performance) that would take as argument the dummy value for the nodes ? That would have the same performance and avoid the use of Obj.

[lpw25]

That seems fine too. My objections were to the automatic rejection of using Obj and to the style of using Obj.magic between concrete types rather than working within Obj.t.

[gretay-js]

Why not use a functor (inlined with annotations if you're worried about performance) that would take as argument the dummy value for the nodes ?

The types stored in this doubly-linked list are instructions and labels, which don't have an obvious dummy value. The previous version used id -1 for instruction id which should never be generated, but it is not ideal to rely on such an invariant.

[Gbury]

I still think that every use of unsafe functions from Obj should be strongly motivated, that is to say, they require:

• that the performance requirement be explicitly stated (in the PR description and/or the code, so that reviewers and/or future developers can now about it), and
• an explanation of why the code cannot be written with the same performance profile without using Obj (likely with a brief summary of what other options exist and why they don't work)

[Gbury]

Why not use a functor (inlined with annotations if you're worried about performance) that would take as argument the dummy value for the nodes ?

The types stored in this doubly-linked list are instructions and labels, which don't have an obvious dummy value. The previous version used id -1 for instruction id which should never be generated, but it is not ideal to rely on such an invariant.

Using an id of -1 to create a dummy instruction seem much more safe than using Obj, so i'm in favour of that option.

[Gbury]

And for labels, you can create a first label at tolevel (for instance using 0 since they appear to be numbers), use that as dummy, and then only generate labels starting from 1, that seems reasonable.

[xclerc]

I got confused and mistakenly pushed another version
to the other PR.

[Gbury]

Why not use a functor (inlined with annotations if you're worried about performance) that would take as argument the dummy value for the nodes ?

The types stored in this doubly-linked list are instructions and labels, which don't have an obvious dummy value. The previous version used id -1 for instruction id which should never be generated, but it is not ideal to rely on such an invariant.

Additionally, except if I missed something, you never actually read/use the value of the dummy node, so there is no requirement at all on the value you use for the dummy node[1], any value of the correct type will work. That value will stay alive forever, but since you never actually use it, it's not even a problem if at some point later you store that very same value in a doubly linked list.

[1] in some cases it's useful/needed to require such a dummy value to be different (either physically or through a compare/equal function) from all other "normal" values, but in the cas of doubly linked list, I could not find a place in the code where that would be needed.

[xclerc]

My understanding is that we will: apply Leo's suggestion,
merge this pull request, have an immediate follow-up
pull request move the module to utils/, and leave any
representation changes to further pull requests.

[gretay-js]

My understanding is that we will: apply Leo's suggestion, merge this pull request, have an immediate follow-up pull request move the module to utils/, and leave any representation changes to further pull requests.

sounds good to me.

@Gbury would it be okay with you to continue this discussion on a separate PR?

[Gbury]

Did you run any benchs/tests to have some idea of the performance improvements regarding time and/or allocations (even rough percentages) ? Using Obj always presents a risk (as we've seen time and time again with bugs appearing with flambda), so it'd be nice to at least have an idea of what the benefits are.

[Gbury]

So, I actually went ahead and modified the implementation of doubly-linked lists to not use Obj (and not need functor either), and opened #1145 with the implementation. If you have the time, it's be interesting to test that implementation to see whether it has the same performance profile as the current code for this PR.

[gretay-js]

So, I actually went ahead and modified the implementation of doubly-linked lists to not use Obj (and not need functor either), and opened #1145 with the implementation. If you have the time, it's be interesting to test that implementation to see whether it has the same performance profile as the current code for this PR.

@Gbury
Thank you for implementing this version so quickly. I agree that #1145 is easier to reason about than the version that uses Obj.magic, but I expect that the extra indirection and extra memory per node in #1145 would have performance impact for very large functions, compared to #1124 . I agree that we should measure it against #1124. Unfortunately, by far the easiest way for us to measure on a meaningful code base at the moment is to merge #1124 first. I would also like to merge a separate
followup PR, extracting DoublyLinkedList out of Cfg. I hope it would help us coordinate, instead of having 3 version of doubly linked list implementation spread across different PRs that include other changes. I am afraid it means you would need to rebase #1145 onto main and move the changes across to a different fiile, I hope it is not too much work.

[Gbury]

@gretay-js I don't see to what indirection you're referring to. #1145 uses an inline record for nodes, which means that the memory layout of a node with it is exactly the same as with this PR, with the only difference begin that pointers to the dummy node are replaced by a tagged integer( for the Empty constructor).

[gretay-js]

@gretay-js I don't see to what indirection you're referring to. #1145 uses an inline record for nodes, which means that the memory layout of a node with it is exactly the same as with this PR, with the only difference begin that pointers to the dummy node are replaced by a tagged integer( for the Empty constructor).

Indeed, I missed it! That's great, thank you.