Caching optimized IR

[cameel]

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.

[ekpyron]

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:

1. it changes how the optimizer is invoked and the caching in principle could be buggy in some way, so having a log entry makes it easier to trace down any change in behavior we may have missed due to this (even though this is not likely)
2. while the change is technically transparent, it should in fact be noticeable in performance, so doesn't hurt to point towards it.

[cameel]

I'd give this one a changelog entry by the way.

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).

[cameel]

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:

1. 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.
2. Do not use cache for pure Yul (my initial idea, but if it's reproducible at Solidity level then it's a no-go).
3. Store the original source along with the cached object.
4. 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.

[cameel]

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.

[cameel]

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.

[cameel]

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.

[clonker]

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.

[cameel]

Regarding testing I think unit tests would be actually good in this case to ensure that the cache functions as intended.

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 CompilerStack, from Solidity rather than Yul and only counts how many objects where cached. It does not exhaustively test the cache functionality with different settings, but we'll at least notice if bytecode dependencies stop being cached. In the end that's the most relevant use case.

I also added some more objectCompiler tests, especially to cover differences in debug data. I actually found a bug in the PR that way - AsmPrinter does not print the @use-src comment so debug data was incomplete and objects with identical code but different debug data were being unified. Now that's fixed.

This is not perfect coverage, but given how simple the cache is, it's probably good enough.

[cameel]

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.

[ekpyron]

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?

[ekpyron]

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?

[ekpyron]

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.

[cameel]

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.

[cameel]

Note that the way it is now, I am caching the object with wrong nativeLocations, 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.

[ekpyron]

Can be merged after another rebase.