Refactor stack classes #1763
Refactor stack classes #1763
Conversation
xclerc
force-pushed
the
refactor-stack-classes
branch
from
c3104e1 to
e9fa6fe
Compare
| @@ -338,7 +338,8 @@ let compile_fundecl ~ppf_dump ~funcnames fd_cmm = | |||
| (* Skip DWARF variable range generation for complicated functions | |||
There was a problem hiding this comment.
Unrelated to this PR, we should factor this out into a helper function (I missed it when it was merged, but perhaps we can sneak it in here since the code is changed?).
| val total_size_in_bytes_for_class : int t -> stack_class:stack_class -> int | ||
|
|
||
| val total_size_in_bytes : int t -> int |
There was a problem hiding this comment.
What are these for? They don't seem to fit in with the generic table.
backend/stack_class.mli
Outdated
| val offset_in_bytes_for_class : stack_class:t -> slot:int -> int | ||
|
|
||
| val offset_in_bytes : int Tbl.t -> stack_class:t -> slot:int -> int |
There was a problem hiding this comment.
I don't understand these two functions: what is the difference between them, why do we need both?
What does int Tbl.t represent - can it be abstracted or named to make this more intuitive?
From the way it is used, I am guessing it is meant for num_stack_slots.
backend/reloadgen.mli
Outdated
| @@ -22,6 +22,6 @@ class reload_generic : object | |||
| method makereg : Reg.t -> Reg.t | |||
| (* Can be overridden to avoid creating new registers of some class | |||
| (i.e. if all "registers" of that class are actually on stack) *) | |||
| method fundecl : Mach.fundecl -> int array -> Mach.fundecl * bool | |||
| method fundecl : Mach.fundecl -> int Stack_class.Tbl.t -> Mach.fundecl * bool | |||
There was a problem hiding this comment.
Would it make sense to label this argument? Something like num_stack_slots? Or perhaps the type can be more informative without a label, something lke Stack_class.num_stack_slots? This appears in a couple of other places, reload.mli.
backend/reloadgen.ml
Outdated
| && Stack_class.equal | ||
| (Stack_class.of_machtype arg.(0).typ) | ||
| (Stack_class.of_machtype res.(0).typ) then |
There was a problem hiding this comment.
This expression appears in a few places, maybe it deserves a helper function, something like Stack_class.equal_of_mach_types and an explanation that different mach types can share stack classes if their slots are of the same size, e.g., Val and Int?
| let offset_in_bytes | ||
| : int Tbl.t -> stack_class:t -> slot:int -> int | ||
| = fun tbl ~stack_class ~slot -> | ||
| match stack_class with |
There was a problem hiding this comment.
| match stack_class with | |
| match stack_class with | |
| (* Preserves original ordering (int -> float) *) |
but really this is the only remaining place where something important is implicit (the layout of slots), so it at least deserves a comment. Considering the upcoming vec256 and friends, it may be worth having a little loop here instead of the match, to sum up total_size_in_bytes_for_class for all classes prior to stack_class in the layout.
| let copy : 'a t -> 'a t = fun tbl -> Tbl.copy tbl | ||
|
|
||
| let copy_values : from:'a t -> to_:'a t -> unit = | ||
| fun ~from ~to_ -> |
There was a problem hiding this comment.
it may be worth having an assert that from and to have the same keys.
| { stack_slots; num_stack_slots } | ||
|
|
||
| let[@inline] size_for_all_stack_classes t = | ||
| Array.fold_left t.num_stack_slots ~f:( + ) ~init:0 | ||
| Stack_class.Tbl.fold t.num_stack_slots |
There was a problem hiding this comment.
Not from this PR but now size_for_all_stack_classes can be more confusing with Stack_class.Tbl.total_size. Maybe rename this to something like total_num_of_stack_slots_for_all_classes?
xclerc
force-pushed
the
refactor-stack-classes
branch
from
e9fa6fe to
7d81e99
Compare
|
Superseded by #3784. |
This pull request refactors the handling of
stack classes. Since the introduction of
vector types (#1499), we distinguish
between register and stack classes.
Having both encoded as mere ints is
error-prone, so this pull request introduces
a proper and abstract type for stack
classes (a similar refactoring will be
performed on register classes, but it is
slightly more complex).
This pull request introduces the following
modules:
Stack_class_utilswhich defines whatstack classes and tables with stack classes
as keys are;
Stack_classwhich defines backend-dependentstack classes.
As of 23c8124, tables are implemented
as hash tables instead of arrays. If this causes
a performance or marshalling issue, it is
easy to use a bare array as the underlying
representation of the table.