-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
[WIP] Expose OpenCL optimization pass #1353
base: master
Are you sure you want to change the base?
Changes from 5 commits
0875110
a9f6bcb
4209a23
5e7a36b
1348044
d261c5b
4048069
93dbe06
9c30349
e4aa467
d0de469
1458491
4ed4daa
edc825b
d1bf03d
0d10d87
911d457
0adfd16
b44de03
51b3ba9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,13 @@ let apply ~default ~merge op (ind : 'a t) = | |
| Between (expr_top, expr_bottom) -> merge (op expr_top) (op expr_bottom) | ||
| MultiIndex exprs -> op exprs | ||
|
||
let map_expr ~f = function | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already derive There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this needs deleted |
||
| All -> All | ||
| Single ind_expr -> Single (f ind_expr) | ||
| Upfrom ind_expr -> Upfrom ind_expr | ||
| Between (expr_top, expr_bottom) -> Between (f expr_top, f expr_bottom) | ||
| MultiIndex exprs -> MultiIndex (f exprs) | ||
|
||
let folder (acc : string Set.Poly.t) op (ind : 'a t) : string Set.Poly.t = | ||
match ind with | ||
| All -> acc | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,4 @@ | ||
open Core_kernel | ||
open Core_kernel.Poly | ||
|
||
(** | ||
* This type represents whether or not an autodiff type can be represented | ||
|
@@ -11,13 +10,18 @@ open Core_kernel.Poly | |
* (fyi a var in the C++ code is an alias for var_value<double>) | ||
* | ||
**) | ||
type t = AoS | SoA [@@deriving sexp, compare, map, hash, fold, equal] | ||
type t = AoS | SoA | OpenCL [@@deriving sexp, compare, map, hash, fold, equal] | ||
|
||
let pp ppf = function | ||
| AoS -> Fmt.string ppf "AoS" | ||
| SoA -> Fmt.string ppf "SoA" | ||
| OpenCL -> Fmt.string ppf "OpenCL" | ||
|
||
let is_soa mem = match mem with SoA -> true | _ -> false | ||
let is_aos mem = match mem with AoS -> true | _ -> false | ||
let is_opencl mem = match mem with OpenCL -> true | _ -> false | ||
Comment on lines
+13
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a relationship between these right? All "OpenCL" memory layouts are automatically SoA, right? Should There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, but I think we should keep them different. For instance like printing the cpp we probably want to know the difference. I need to think about a right way to describe this |
||
|
||
let lub_mem_pat lst = | ||
let find_soa mem_pat = mem_pat = SoA in | ||
let find_soa mem_pat = match mem_pat with SoA -> true | _ -> false in | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Equivalent to |
||
let any_soa = List.exists ~f:find_soa lst in | ||
match any_soa with true -> SoA | false -> AoS |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -190,7 +190,7 @@ let rec get_mem_pattern st = | |
| SVector (mem, _) | SRowVector (mem, _) | SMatrix (mem, _, _) -> mem | ||
| SArray (t, _) -> get_mem_pattern t | ||
|
||
(*Given a sizedtype, demote it's mem pattern from SoA to AoS*) | ||
(*Given a sizedtype, demote it's mem pattern from SoA or OpenCL to AoS*) | ||
let rec demote_sizedtype_mem st = | ||
match st with | ||
| ( SInt | SReal | SComplex | ||
|
@@ -201,12 +201,12 @@ let rec demote_sizedtype_mem st = | |
| SComplexMatrix (_, _) ) as ret -> | ||
ret | ||
| SArray (inner_type, dim) -> SArray (demote_sizedtype_mem inner_type, dim) | ||
| SVector (SoA, dim) -> SVector (AoS, dim) | ||
| SRowVector (SoA, dim) -> SRowVector (AoS, dim) | ||
| SMatrix (SoA, dim1, dim2) -> SMatrix (AoS, dim1, dim2) | ||
| SVector ((SoA | OpenCL), dim) -> SVector (AoS, dim) | ||
| SRowVector ((SoA | OpenCL), dim) -> SRowVector (AoS, dim) | ||
| SMatrix ((SoA | OpenCL), dim1, dim2) -> SMatrix (AoS, dim1, dim2) | ||
| STuple subtypes -> STuple (List.map ~f:demote_sizedtype_mem subtypes) | ||
|
||
(*Given a sizedtype, promote it's mem pattern from AoS to SoA*) | ||
(*Given a sizedtype, promote it's mem pattern from AoS to SoA *) | ||
let rec promote_sizedtype_mem st = | ||
match st with | ||
| SVector (AoS, dim) -> SVector (SoA, dim) | ||
|
@@ -220,6 +220,15 @@ let modify_sizedtype_mem (mem_pattern : Mem_pattern.t) st = | |
match mem_pattern with | ||
| AoS -> demote_sizedtype_mem st | ||
| SoA -> promote_sizedtype_mem st | ||
| OpenCL -> promote_sizedtype_mem st | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OpenCL is "promoted" to SoA by this function? |
||
|
||
let rec promote_mem (mem_pattern : Mem_pattern.t) st = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like it should be called |
||
match st with | ||
| SVector (_, dim) -> SVector (mem_pattern, dim) | ||
| SRowVector (_, dim) -> SRowVector (mem_pattern, dim) | ||
| SMatrix (_, dim1, dim2) -> SMatrix (mem_pattern, dim1, dim2) | ||
| SArray (inner_type, dim) -> SArray (promote_mem mem_pattern inner_type, dim) | ||
| _ -> st | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to consider things inside tuples? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want to allow OpenCL to use tuples then yes. Also the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, that’s too bad. You could do something like what we had to do for Autodiff level and have a tuple specific variant in the type, but I wasn’t super happy with that either. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah eod I think it's going to look a little odd in some places but I think it's fine to just add it to the sized types |
||
|
||
let rec has_mem_pattern = function | ||
| SInt | SReal | SComplex | SComplexVector _ | SComplexRowVector _ | ||
|
@@ -229,6 +238,16 @@ let rec has_mem_pattern = function | |
| SArray (t, _) -> has_mem_pattern t | ||
| STuple subtypes -> List.exists ~f:has_mem_pattern subtypes | ||
|
||
let is_eigen_type st = | ||
match st with | ||
| (SVector (mem, _) | SRowVector (mem, _) | SMatrix (mem, _, _)) | ||
when Mem_pattern.is_opencl mem -> | ||
false | ||
| SVector _ | SRowVector _ | SMatrix _ | SComplexRowVector _ | ||
|SComplexVector _ | SComplexMatrix _ -> | ||
true | ||
| _ -> false | ||
|
||
Comment on lines
+253
to
+262
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel a little nervous that What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I agree it's very shaky. I think splitting it out it a better idea |
||
(** The inverse of [get_array_dims] | ||
*) | ||
let build_sarray dims st = | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd want this to be enabled as part of
all_optimizations
so this should beb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk, personally this is a very specific optimization for a piece of hardware. I'd rather the user explicitly flips it on