-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Caching optimized IR #15267
Caching optimized IR #15267
Conversation
I'd give this one a changelog entry by the way. One could argue that it doesn't need one, since the change should be purely transparent - but I'd have two reasons for still giving it an entry:
|
Yeah, definitely. I was going to do that anyway, just still fighting with a different issue (getting different source locations for pure Yul compilation - may have to disable caching for that). |
2234c5a
to
41754f3
Compare
libyul/ObjectCache.cpp
Outdated
bool _isCreation | ||
) | ||
{ | ||
AsmPrinter asmPrinter(nullptr, _debugData.sourceNames, DebugInfoSelection::All()); |
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 failing tests for pure Yul compilation are caused by this. Without providing the original source the debug info does not get printed. This shows up most easily for Yul since the source locations are then pointing at Yul source but I think it should be reproducible at Solidity level too.
I can see a few solutions:
- Adjust asm printer to make it possible to print locations even without source available. This should be possible as long as we don't request the snippets.
- Do not use cache for pure Yul (my initial idea, but if it's reproducible at Solidity level then it's a no-go).
- Store the original source along with the cached object.
- Switch to calculating cache key directly from AST and account for debug info properly, directly in the form it's stored.
If I can't do 2., then I think 1. is the way to go here. Still relatively quick to do, not a hack and keeps the design reasonable.
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.
Turns out that it wasn't caused by not providing sources. It was instead the fact that Object::debugData
is empty if the initial Yul input does not have the @use-src
annotation. AsmPrinter
does not print source locations in that case. I solved the problem by simply disabling caching for such objects. Caching for pure Yul is not the primary use case for this feature anyway and also IR produced by the compiler always has the annotation.
A better solution would be if we could ensure that AsmPrinter
always prints all the debug info it has, but when I tried that it resulted with pretty verbose debug annotations being to Yul source that originally did not have them. I need to dig more into this to check why and if we can work around that. But that's out of scope of this PR.
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.
We discussed it today and the conclusion was that locations present in debug info when @use-src
is missing are wrong. They should be from optimized source, not from unoptimized. If that was the case they would not be changing depending on where the unoptimized object was originally located in the source.
I actually fixed that exact problem recently but only for CompilerStack
, not pure Yul. Now I have a more comprehensive fix: #15309. I rebased the PR on top of it and I can confirm that this fixes the problem for objects that do not have @use-src
.
24e5095
to
7e65f87
Compare
I'm marking it as ready to review, since the implementation is done, but I'm not very happy with test coverage, so I'll probably add some more tests tomorrow. A big problem is that it's hard to create a test that checks that things are really being cached. I actually broke it at some point and only realized when I reran benchmarks, which we don't do in CI. |
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.
From looking at the code I only found a few superficial things. Regarding testing I think unit tests would be actually good in this case to ensure that the cache functions as intended.
7e65f87
to
e67b884
Compare
Unfortunately proper unit testing is not easy here either. At least not without adding otherwise unused functionality to inspect the workings of the cache and storing extra data in it. Otherwise too little information is exposed. E.g. we don't really know what is included in what key or which objects in a hierarchy were restored from cache. In the end I settled on adding a new type of a test case. It only really tests the usage via I also added some more This is not perfect coverage, but given how simple the cache is, it's probably good enough. |
8caa355
to
ba5b5ea
Compare
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.
Note that the main point of these tests is to verify the introducing the cache does not change the output. I put the result of running them on develop
in a separate commit. They still pass without modifications on top of the cache.
Other than that they're still useful to have overall as regression tests for Yul compilation/optimization.
// All objects have identical unoptimized code, but with different debug annotations. | ||
// After optimizations we should end up with identical code for all of them, because | ||
// debug info will be stripped due to missing @use-src annotations. |
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.
How do you justify this behavior?
This seems very problematic - I thought you had a version in which this kind of thing didn't happen in the end?
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 guess the description is just bad in this comment - the point is that the sources here do not have valid source location comments, since they will never be parsed and used without a @use-src
annotation, right?
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.e. there is nothing stripped here, there aren't any valid source locations in the first place, so they don't affect the caching is rather what's happening, I'd say.
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 just a matter of point of view. I was thinking about the comments in the source and apparently you're thinking about debug data in the AST instead.
I meant that location comments are not the same in the sources. I.e. the objects don't all contain the @src
annotation. I guess if you're only looking at what ends up in the parsed AST then yeah, you could say there's no debug info.
Similarly, the original source contains location comments and optimized IR does not. So from that point of view they get stripped.
Anyway, I reworded this to say "location comments" rather than "debug info" when I specifically mean what's in the source. Hope that makes things clearer.
ba5b5ea
to
b7310a3
Compare
b7310a3
to
9b7927d
Compare
c161d8b
to
3d71d8f
Compare
// NOTE: AsmPrinter never prints nativeLocations included in debug data, so ASTs differing only | ||
// in that regard are considered equal here. This is fine because the optimizer does not keep | ||
// them up to date across AST transformations anyway so in any use where they need to be reliable, | ||
// we just regenerate them by reparsing the object. | ||
rawKey += keccak256(asmPrinter(_ast)).asBytes(); |
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.
Note that the way it is now, I am caching the object with wrong nativeLocation
s, so when it's retrieved from the cache those are still be wrong. I expect that the user of the cache (i.e. YulStack
) will reparse the source to correct the problem.
It would be a better to have it encapsulated here, so that the object is reparsed after it's optimized but before it's cached. Unfortunately to do this I'd have to duplicate the parsing and analyzing logic from YulStack
here or do some refactor to share it so in the end I thought this was a better trade-off.
0d75360
to
babe3d9
Compare
3d71d8f
to
15643bb
Compare
babe3d9
to
fe4c52e
Compare
15643bb
to
115757a
Compare
Can be merged after another rebase. |
115757a
to
33c9cc3
Compare
33c9cc3
to
5c7fc04
Compare
Fixes #15179.
Replaces #15182, #15230.
Depends on #15309.
This is yet another attempt at reusing optimized IR. Simpler than the previous ones.
I still need to iron out a few minor things and clean it up so it's still a draft, but at this point it's mostly done.Done.