Extract expressionizer and fix optimizer. #2056
Conversation
chriseth
changed the title
There was a problem hiding this comment.
This fixes #2051, nice!
I reviewed everything but the first commit (the diff is pretty big, also when comparing the old condenser with the expressionizer file, but I believe you that you just moved code...). I have a few comments on how I think we can simplify the code a bit.
| let ((_, array_start), symbol_name) = self | ||
| .poly_id_to_name | ||
| .range(..=(poly_id.ptype, poly_id.id)) | ||
| .last() | ||
| .unwrap(); |
There was a problem hiding this comment.
Do I understand this right:
poly_id_to_namewould have an entry for the array, but not each element- This code finds it's
PolyID.id(which would that of the first element), assuming a consecutive ordering of the IDs of an array.
I think this deserves a comment :)
| } | ||
|
|
||
| struct Expressionizer<'a> { | ||
| poly_id_to_name: &'a BTreeMap<(PolynomialType, u64), String>, |
There was a problem hiding this comment.
(PolynomialType, u64) is essentially PolyID, except that it stores the type first, which makes the range thing work, right? In that case, could we just change PolyID to store the type first?
But on a higher-level note, why don't we have something like array_elements: BTreeMap<PolyID, (String, usize)> (if a polynomial is an array element, maps it to the array name and index) instead? That would get rid of the poly_id_to_name.range(...).last() thing below, which seems kinda hacky...
There was a problem hiding this comment.
Especially since, in my understanding, the array_elements map would not change during the condenser run, would it? I feel like this could simplify some code further.
There was a problem hiding this comment.
I thought about changing PolyID but decided against because it's too fragile. We would need a static assert here to prevent other changes to PolyID from breaking the assumptions here.
I don't want to store an element for every single array element because I wanted to avoid performance problems coming from using large arrays.
The condenser run can actually add new arrays any time and we need to be able to handle them.
There was a problem hiding this comment.
I thought about changing
PolyIDbut decided against because it's too fragile. We would need a static assert here to prevent other changes toPolyIDfrom breaking the assumptions here.
Agreed.
The condenser run can actually add new arrays any time and we need to be able to handle them.
Right, I didn't see that Condenser::new_column (the only place that changes Condenser::poly_id_to_name) can actually add arrays of columns as well.
I don't want to store an element for every single array element because I wanted to avoid performance problems coming from using large arrays.
We are only talking about arrays of columns, aren't we? So at most, they will be in the 1000s, because otherwise we'd have other problems, right?
|
Added some comments. |
| } | ||
|
|
||
| struct Expressionizer<'a> { | ||
| poly_id_to_name: &'a BTreeMap<(PolynomialType, u64), String>, |
There was a problem hiding this comment.
I thought about changing
PolyIDbut decided against because it's too fragile. We would need a static assert here to prevent other changes toPolyIDfrom breaking the assumptions here.
Agreed.
The condenser run can actually add new arrays any time and we need to be able to handle them.
Right, I didn't see that Condenser::new_column (the only place that changes Condenser::poly_id_to_name) can actually add arrays of columns as well.
I don't want to store an element for every single array element because I wanted to avoid performance problems coming from using large arrays.
We are only talking about arrays of columns, aren't we? So at most, they will be in the 1000s, because otherwise we'd have other problems, right?
Co-authored-by: Georg Wiese <[email protected]>
No description provided.