Apply transforms as part of optimization passes #139
Conversation
|
PS: Do you know of a good way to specify these passes as part of some input configuration? Or is this not useful? |
|
(addresses #134) |
|
You could probably make a table of all the transforms, which maps a string name to each transform. That's similar to what clang does too (e.g., you can pass something like |
|
This is great as a start but I'd consider creating a Pass class that has a name, etc. Then we can print nicer debugging info about passes or disable particular ones. |
|
Also forgot to note that I plan on adding functionality that turns passes on and off in a separate PR... |
|
Actually just added the ability to specify optimization passes as part of the config. This works from Python too. |
weld/passes.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn get_pass(pass_name: String) -> WeldResult<Pass> { |
There was a problem hiding this comment.
Why is pass_name a String instead of &str?
weld/passes.rs
Outdated
| } | ||
|
|
||
| pub fn get_pass(pass_name: String) -> WeldResult<Pass> { | ||
| match pass_name.as_ref() { |
There was a problem hiding this comment.
Might be nicer to put these in a map with lazy_static.
weld/passes.rs
Outdated
|
|
||
| pub struct Pass { | ||
| transform_fns: Vec<fn(&mut Expr<Type>)>, | ||
| transform_names: Vec<String>, |
There was a problem hiding this comment.
This is kind of hard to maintain because the vectors have to be the same length; I'd create a separate "Transformation" type that has both a name and a function, or drop the names.
|
@mateiz, addressed. Let me know if anything looks amiss. |
weld/passes.rs
Outdated
| m.insert("inline-apply", Pass { | ||
| transform_fns: vec![transforms::inline_apply], | ||
| pass_name: String::from("inline-apply") | ||
| }); |
There was a problem hiding this comment.
Nit: I'd consider cleaning these up by creating a Pass::new constructor. For example, You could then do m.insert("inline-apply", Pass::new("inline_apply, vec![inline_apply])).
weld/passes.rs
Outdated
|
|
||
|
|
||
| pub struct Pass { | ||
| transform_fns: Vec<fn(&mut Expr<Type>)>, |
There was a problem hiding this comment.
Could just name this "transforms"
weld/conf.rs
Outdated
| pub fn parse_optimization_passes(s: CString) -> Vec<String> { | ||
| let s = s.into_string().unwrap(); | ||
| match s.as_ref() { | ||
| "" => DEFAULT_OPTIMIZATION_PASSES.to_vec().iter().map(|e| e.to_string()).collect(), |
There was a problem hiding this comment.
Maybe DEFAULT_OPTIMIZATION_PASSES should be a static Vec<String> and we should just clone it. Not sure whether that's easier.
|
Looks OK overall, but I just made a few small comments on things that may make it clearer. |
|
BTW feel free to just merge it once you've fixed those. |
No description provided.