optimizer: deduplicate fixed columns #2102
Conversation
pilopt/src/lib.rs
Outdated
| @@ -215,6 +217,39 @@ fn constant_value(function: &FunctionValueDefinition) -> Option<BigUint> { | |||
| } | |||
| } | |||
|
|
|||
| /// Deduplicate fixed columns which share the same value. | |||
| /// This uses the `Display` implementation of the function value, so `|i| i` is different from `|j| j` | |||
There was a problem hiding this comment.
Is it that much more complicated to check for syntactic equality? I think Display is very expensive.
There was a problem hiding this comment.
I can have a go at it, seemed annoying because of sourcemaps.
Schaeff
force-pushed
the
deduplicate-fixed-columns
branch
2 times, most recently
from
02f041a to
d0a95ff
Compare
Schaeff
force-pushed
the
deduplicate-fixed-columns
branch
from
eca4b56 to
19e31ac
Compare
| @@ -215,6 +217,44 @@ fn constant_value(function: &FunctionValueDefinition) -> Option<BigUint> { | |||
| } | |||
| } | |||
|
|
|||
| /// Deduplicate fixed columns of the same namespace which share the same value. | |||
| /// This compares the function values, so `|i| i` is different from `|j| j` | |||
There was a problem hiding this comment.
We could actually quite easily support |i| i being equal to |j| j, because all references are already the same. The only thing we need to change is the comparison and hash function for Pattern to ignore the variable name.
There was a problem hiding this comment.
Oh great, I was hoping for something like this
There was a problem hiding this comment.
So I'm looking into this, I don't think we should make all Pattern::Variable equal, right?
One level up I was looking at
#[derive(
Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize, JsonSchema, Hash,
)]
pub struct LambdaExpression<E = Expression<NamespacedPolynomialReference>> {
pub kind: FunctionKind,
pub params: Vec<Pattern>,
pub body: Box<E>,
/// Type of the parameters, filled in during type inference.
pub param_types: Vec<Type>,
}Here I would expect body to refer to the parameters by index, in which case it would be the same no matter what the parameters are called. Then we could ignore them in comparisons.
Unless I'm missing something obvious, this seems like it should be a different PR?
There was a problem hiding this comment.
I mean the question is if we want to do that in the "default" comparison function. Is there an easy way to provide a different comparison - probably quite cumbersome to do that recursively, right?
There was a problem hiding this comment.
Opened #2128 for this
pilopt/src/lib.rs
Outdated
| // group symbols by common namespace and function value | ||
| .into_group_map_by(|(symbol, value)| { | ||
| ( | ||
| symbol.absolute_name.split("::").next().unwrap(), |
There was a problem hiding this comment.
Use SymbolPath::name or at least introduce a helper function somewhere?
There was a problem hiding this comment.
Ah sorry, this is the namespace, not the name.
| }) | ||
| .collect(); | ||
|
|
||
| // substitute all occurences in expressions. |
There was a problem hiding this comment.
I think you also have to substitute in definitions. This has some overlap with #2086.
There was a problem hiding this comment.
Now substituting also in definitions, however I'm struggling to think of an example where that would apply, the ones I came up with were being optimised away at a previous step.
pilopt/src/lib.rs
Outdated
| .into_group_map_by(|(symbol, value)| { | ||
| ( | ||
| symbol.absolute_name.split("::").next().unwrap(), | ||
| value.as_ref().unwrap(), |
There was a problem hiding this comment.
I think the parser also allows fixed columns without value, but I don't know how far we advance for those.
There was a problem hiding this comment.
It does not seem like this is something we expect, so it would be fine to fail if it does not fail before?
There was a problem hiding this comment.
Maybe we should remove that option from the parser.
With bus lookups, we insert
is_firstonce per lookup.Only keep a single column per syntactic value.
Depends on #2103