Conversation
45dc45c to
aabdfa9
Compare
830857a to
5f80ae7
Compare
|
Also, @chriseth, do you have a suggestion for the changelog entry? |
88545c5 to
f759e94
Compare
f79d84e to
b6a2886
Compare
b6a2886 to
232d967
Compare
|
|
||
| /** | ||
| * Erase entries in both maps based on provided ``_variable``. | ||
| * For example, after deleting ``c`` for ``Ref 1``, ``Ref 1`` would contain the following: |
There was a problem hiding this comment.
Can we say that erase(x) is exactly the same as set(x, {})?
There was a problem hiding this comment.
Eh, yes and no - since we do m_ordered[_variable] = _references;, which will still make an insertion for key x. I don't think it would ultimately affect the behaviour. I could insert an empty check for _references however, in which case your statement would be fully correct.
edit: Actually, I'm assuming you already knew that this will insert a key with an empty value, so yes, it's exactly the same as set(x, {}).
There was a problem hiding this comment.
I don't get your answer here. You're saying that it's not the same but then that it's the same after all. It doesn't look the same to me.
I could insert an empty check for
_referenceshowever, in which case your statement would be fully correct.
I'd add this check because I don't think we care about distinguishing x being assigned an empty set of variables from not being assigned anything. The former can't even be expressed in the language. With this check the behavior of the container will be more consistent - currently with getOrderedOrNullptr() you have to check for an empty set explicitly, while with getReversedOrNullptr() you can assume you'll always get nullptr instead.
There was a problem hiding this comment.
Actually, now that I think about it, it can be expressed after all. x can just be assigned a constant expression that does not depend on other variables. So yeah, depends on whether we want the ability to express that. Does not seem to me like we're using that distinction for anything currently.
There was a problem hiding this comment.
The edit's the final answer - i.e. yes, it's exactly as Chris suggested; adding the empty _references check would alter the behaviour of the analysis (I would assume we'd see failing tests, but I'd have to check). I.e. we fetch by key, and then use the value set to either perform arithmetic (i.e. add two sets together), or lookup, neither of which need an empty set check.
There was a problem hiding this comment.
ok, you're right about this altering the analysis.
But I'm still confused as to why you think erase(x) and set(x, {}) would be equivalent. This just does not seem true to me. Are you referring to the fact that m_ordered[_variable] will modify m_ordered and insert the key if it's not there? You're still doing m_ordered.erase() at the end of the function so yeah, it will technically insert the key but the key won't be there when the function finishes. So I don't think it's true that The behaviour is the same as ``set("x", {})``.. This bit should be removed from the docstring unless I'm missing something here.
232d967 to
80454c1
Compare
|
Looks good! Please squash. |
80454c1 to
a8cc9bd
Compare
Done! |
| @@ -0,0 +1,39 @@ | |||
| #include <libyul/optimiser/VariableAssignmentMap.h> | |||
There was a problem hiding this comment.
Something felt off to me about this file and I finally realized why. It looks too clean. We can't have such nice things here :P You must add the ugly license boilerplate.
| * Class that implements a reverse lookup for an ``unordered_map<YulString, set<YulString>>`` by wrapping the | ||
| * two such maps - one ordered, and one reversed, e.g. | ||
| * | ||
| * m_ordered m_reversed |
There was a problem hiding this comment.
This naming is a bit confusing. What specifically is ordered here? Its type is unordered_map so it surely can't be referring to the order of elements, can it?
Maybe something like m_assignments and m_uses would be better names? Or maybe m_lValues and m_rValues?
There was a problem hiding this comment.
It's ordered in the sense that it's the opposite of reversed. m_assigments and m_uses isn't really correct either, e.g. if we have
a = x + y;
then (a,x) and (a,y) could be assignments - but what are the uses? (x, a), (y, a)? That doesn't really make sense to me either. lvaluesandrvalues` do make sense, but are then somewhat confusing in terms of C++ semantics. In any case, I've spend quite a while trying to come up with names for these, and I'm still convinced these are the best, especially since the whole purpose of this PR is to implement a reverse lookup.
| * | ||
| * m_ordered m_reversed | ||
| * x -> (y, z,) y -> (x,) | ||
| * y -> (z,) |
There was a problem hiding this comment.
| * y -> (z,) | |
| * z -> (x,) |
|
|
||
| /** | ||
| * Erase entries in both maps based on provided ``_variable``. | ||
| * For example, after deleting ``c`` for ``Ref 1``, ``Ref 1`` would contain the following: |
There was a problem hiding this comment.
I don't get your answer here. You're saying that it's not the same but then that it's the same after all. It doesn't look the same to me.
I could insert an empty check for
_referenceshowever, in which case your statement would be fully correct.
I'd add this check because I don't think we care about distinguishing x being assigned an empty set of variables from not being assigned anything. The former can't even be expressed in the language. With this check the behavior of the container will be more consistent - currently with getOrderedOrNullptr() you have to check for an empty set explicitly, while with getReversedOrNullptr() you can assume you'll always get nullptr instead.
| if (names.count(variableToClear)) | ||
| _variables.emplace(ref); | ||
| if (auto&& references = m_state.references.getReversedOrNullptr(variableToClear)) | ||
| _variables += *references; |
There was a problem hiding this comment.
Are you sure getReversedOrNullptr() will never return nullptr here? Seems like it should happen any time we have a variable that's assigned to and then never used. If this does not crash on a null dereference then perhaps we don't have any case like that in tests? Sounds unlikely though.
Please either handle nullptr here or add an assert.
By the way, having two completely different things named references here makes this bit unnecessarily confusing to read.
There was a problem hiding this comment.
It's handled already (see if condition). What do you mean by having two things named references? auto&& references and m_state.references?
There was a problem hiding this comment.
It's handled already (see
ifcondition).
Ah, you're right. That's why I'm not really sold on this if-declaration syntax personally. It can be convenient but sometimes it just does not register as a proper condition when I read it :)
What do you mean by having two things named
references?auto&& referencesandm_state.references?
Yes.
| // Also clear variables that reference variables to be cleared. | ||
| for (auto const& variableToClear: _variables) | ||
| for (auto const& [ref, names]: m_state.references) | ||
| if (names.count(variableToClear)) | ||
| _variables.emplace(ref); | ||
| if (auto&& references = m_state.references.getReversedOrNullptr(variableToClear)) | ||
| _variables += *references; |
There was a problem hiding this comment.
This whole loop looks suspect to me, both before and after your change. We're inserting new items into _variables while iterating over it. Apparently adding to a set in C++ does not invalidate iterators so this won't crash if it's wrong and we have a finite number of items so it won't fall into an infinite loop either. Still, it looks like it would inconsistently iterate over some added elements while skipping others - depending on whether they sort before or after the current element. Also, iterating over newly added elements seems wrong in the first place because the comment above says we're not supposed to clear variables recursively and that would basically be the result.
I think that clearing too little is more dangerous here than cleaning too little and therefore this does not outright break things. It probably just makes the analyzer less effective instead.
|
|
||
| /** | ||
| * Erase entries in both maps based on provided ``_variable``. | ||
| * For example, after deleting ``c`` for ``Ref 1``, ``Ref 1`` would contain the following: |
There was a problem hiding this comment.
Actually, now that I think about it, it can be expressed after all. x can just be assigned a constant expression that does not depend on other variables. So yeah, depends on whether we want the ability to express that. Does not seem to me like we're using that distinction for anything currently.
| for (auto&& reference: m_ordered[_variable]) | ||
| if (m_reversed.find(reference) != m_reversed.end()) | ||
| { | ||
| if (m_reversed[reference].size() > 1) | ||
| m_reversed[reference].erase(_variable); | ||
| else | ||
| // Only fully remove an entry if no variables other than _variable | ||
| // are contained in the set pointed to by reference. | ||
| m_reversed.erase(reference); | ||
| } | ||
| m_ordered.erase(_variable); |
There was a problem hiding this comment.
There's an easy micro-optimization here - you're looking up reference 3 times, while you could instead just find an iterator to the element and then use that. Similar with _variable - 2 separate lookups.
It's a fixed number of times so it won't change the complexity of the whole algorithm but it's very easy to do. I'm curious if it will make any kind of difference in benchmark results given that we perform this operation a lot. The lookup is something like O(log n) so nothing compared to the linear search we had before but still worth a try.
|
I checked how this PR affects compilation times in external tests.
I wonder what's happening with ElementFi. I double checked I got the numbers from the right pages and looks like it really took ~30 s longer. May be worth checking if it's repeatable or just a fluke. Still, some other times increased too so there might be something more to it. The table contains only the Details |
|
This pull request is stale because it has been open for 14 days with no activity. |
a8cc9bd to
0d1ec65
Compare
|
I gathered fresh timing data (and translated the original data into the same format).
My conclusion here is that there's no strong evidence that this change significantly affects external tests, be it positively or negatively. If there is a difference, it's lower than the normal variance of CI timing. Still, the change does seem to help one especially pathological contract from our repo ( Original timing
Current timing
Timing diff with
|
| Project | Diff (original) | Diff (run 1) | Diff (run 2) | Diff (run 3) |
|---|---|---|---|---|
| brink | 1 s | -1 s | 0 s | 0 s |
| colony | -24 s | 8 s | 5 s | 9 s |
| elementfi | 27 s | -10 s | -9 s | -5 s |
| ens | 3 s | 3 s | 3 s | 7 s |
| euler | 7 s | 10 s | 20 s | |
| gnosis | ||||
| gp2 | -1 s | 6 s | 5 s | 18 s |
| perpetual-pools | 4 s | -9 s | 3 s | -2 s |
| pool-together | -7 s | -5 s | 5 s | |
| uniswap | -6 s | 2 s | -11 s | 2 s |
| yield_liquidator | 7 s | 1 s | 0 s | -1 s |
| zeppelin | -62 s | 5 s | 13 s | 16 s |
| prb-math | -3 s |
Timing benchmark
develop
Binary from b_ubu_static from the last run on develop
| File | Pipeline | Bytecode size | Time | Exit code |
|---|---|---|---|---|
verifier.sol |
legacy | 4940 bytes | 0.20 s | 0 |
verifier.sol |
via-ir | 4417 bytes | 0.66 s | 0 |
OptimizorClub.sol |
legacy | 0 bytes | 0.52 s | 1 |
OptimizorClub.sol |
via-ir | 22391 bytes | 4.05 s | 0 |
chains.sol |
legacy | 5878 bytes | 0.17 s | 0 |
chains.sol |
via-ir | 23076 bytes | 23.16 s | 0 |
this PR
Binary from b_ubu_static from the last run on dataflow-analyzer-reverse-lookup
| File | Pipeline | Bytecode size | Time | Exit code |
|---|---|---|---|---|
verifier.sol |
legacy | 4940 bytes | 0.14 s | 0 |
verifier.sol |
via-ir | 4417 bytes | 0.65 s | 0 |
OptimizorClub.sol |
legacy | 0 bytes | 0.57 s | 1 |
OptimizorClub.sol |
via-ir | 22391 bytes | 4.23 s | 0 |
chains.sol |
legacy | 5878 bytes | 0.17 s | 0 |
chains.sol |
via-ir | 23076 bytes | 10.52 s | 0 |
|
This PR has been open for more than a year, with the improvements somewhat inconclusive. I'll be closing it, at least for the time being, since we're working on different (macro) improvements on the via-ir pipeline, which should yield significantly better results. We can re-open this in the future if need be, or use it as a reference at the very least. |
Part of #13719 and #13822.
Benchmarking is run on the contract.
Mostly helps with CSE (
CommonSubexpressionEleminator) andLiteralRematerialiser,LoadResolverandExpressionSimplifier, i.e. steps that inherit from and thus rely on theDataFlowAnalyzer. The results are as follows:This PR is essentially a rework of @chriseth's Improve DataFlowAnalyzer PR, which includes many other improvements (of which I think all were merged already in separate PRs).
The question here is whether we'd prefer the approach here (where the in-order and reverse lookup are wrapped in a separate class), or in Chris' PR, where the reverse lookup is just added to the
DataFlowAnalyzer::Stateobject and then used directly from there.develop (baseline)
with reverse lookup (this)