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
Reverse lookup for DataFlowAnalyzer #14112
base: develop
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we say that erase(x)
is exactly the same as set(x, {})
?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
_references
however, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
lvaluesand
rvalues` 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
_references
however, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's handled already (see
if
condition).
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&& references
andm_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
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 |
Part of #13719 and #13822.
Benchmarking is run on the contract.
Mostly helps with CSE (
CommonSubexpressionEleminator
) andLiteralRematerialiser
,LoadResolver
andExpressionSimplifier
, 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::State
object and then used directly from there.develop (baseline)
with reverse lookup (this)