Make the hard-coded parts of the optimiser sequence configurable

[ekpyron]

The CSE run at the very end after the stack compressor actively causes problems in https://github.com/ethereum/solidity/issues/9622

So maybe we should think about making the hard-coded part of the sequence suite.runSequence("fDnTOc g", ast);, resp. the earlier suite.runSequence("g", ast); configurable as well.

My idea would have been to add T, the literal materializer to the end of the sequence to avoid the problem, but that's simply undone by the CSE without the possibility of preventing that :-).

I think the main reason for it being done like that is the stack compressor being rather special and needing special care or preservation using the phaser... not sure what's the best solution for this, but it'd be good if this allowed more freedom to configure things if needed.

[cameel]

Here's the gist of my discussion with @ekpyron on Gitter:

The reason that the stack compressor and the g and fDnTOc g parts are hard-coded is that it's just final cleanup and we don't want them to disappear when we use a sequence produced by yul-phaser. This part is not meant to be evaluated using the same criteria as the rest of the sequence (i.e. just taking the size of the resulting code into account).

If Stack Compressor is safe to use at arbitrary points in the sequence (it does not have to be effective but must not crash the compiler), it could be assigned a letter and treated as any other step. I just didn't see any practical need for doing it like that and decided to keep it simple. It would need a tiny wrapper to make it accept the same parameters as other steps and the phaser would also need a special case to ignore the Stack Compressor step. Then the hard-coded part could be made a part of the main sequence.

We would just need to document that the cleanup part and the compressor should be included when using a custom sequence. Or, for better usability, it might be best to have two separate options: one to change the first part of the sequence (meant to optimize size) and the other to change the final cleanup. It would make it easier to override one without having to change the other. Having phaser actually treat it like two sequences and use different criteria is actually something that came up in my discussions with @chriseth while I was working on it.

So implementing it is not a problem, it's just a matter of deciding how configurable we want/need it to be.

[cameel]

We discussed it today and decided that making stack compressor a step and the whole sequence configurable is a good idea. Also, we want to keep it simple and have just a single option for changing the sequence.

[ekpyron]

We should prioritize this, since I think it will help to discuss the IR pipeline with debuggers, since only with this change we can really fine-tine exactly what the optimizer does and doesn't do without intransparent implicit default behaviour.

[cameel]

I think we should actually treat it as a breaking change (unless we decide to add a new option instead of changing this one). Some projects I've seen in the wild are using it to avoid "Stack too deep" errors. We don't guarantee that the optimizer steps we run won't change between releases but this one is a bit more disruptive. It would make anyone using a custom sequence stop getting stack compressor and it might not be easy to figure out why all of a sudden there are so many stack problems :)

[ekpyron]

Ultimately we wanted to get rid of the stack compressor anyways... but even if I find the time and manage to actually have the new evm code transform do that, we still have the problem that it will still have to be invoked and even invoked at a different point when compiling for homestead... I wonder if keeping the second path for homestead is really worth it actually... Or actually... even for newer EVM versions we call it at a different point if stack optimizations are disabled... I think all of this may require some additional thought :-).

[cameel]

I'd stay with the current idea of creating a step just to keep the scope of this task small and manageable :)

BTW, can you say more about how it relates to Homestead? I don't know much about that.

[ekpyron]

BTW, can you say more about how it relates to Homestead? I don't know much about that.

The new code transform is incompatible with homestead, so we use the optimized version of the old code transform for it - and depending on the code transform it's better to call the stack compressor at different stages (the stack compressor is intertwined with the code transform - the code transform basically tells it which variables it should try to eliminate - for the old code transform that works rather heuristically, for the new one it's a very specific minimal set of variables and further steps afterwards may break things again)

But, actually, homestead is only one of two cases in which we run the stack compressor early - the second one is optimized compilation with disabled stack optimization - although I'm not sure that's a reasonable setting we want to keep.

We need to think about how to deal with this in the future - maintaining those separate paths right now is a bit unfortunate, but we haven't decided what to do yet.

[ekpyron]

This may by the way also partly, or even fully, address the allow performing no optimizations other than stack-to-memory part of https://github.com/ethereum/solidity/issues/13157...

[ekpyron]

Also are we sure we have to consider this a breaking change?

[cameel]

Honestly, my biggest concern is not just that it's breaking but that the breakage will likely happen silently. Anyone actually using it will lose some optimizer steps, possibly without even realizing it.

But I have another idea: how about introducing some kind of prefix that will be required to enable the new behavior (for example --yul-optimizations @fDnTOcg) and keeping the current behavior by default until 0.9? Then we'd have no issues with breakage, silent or not.

[cameel]

I discussed it with @ekpyron today and adding a prefix would be fine but https://github.com/ethereum/solidity/issues/9627#issuecomment-1016376145 still leads to some ugly complications in the code. We would either have to parameterize OptimiserSettings on the EVM version and dialect or introduce some conditional syntax into the sequence.

We now lean towards going back to the idea of having a separate "cleanup" sequence. So this way the current option would stay as is (i.e. full backwards compatibility) and we would just introduce another one for the fDnTOc g part.

[ekpyron]

So as for some more guidance: We currently have the command line option -yul-optimizations steps and the standard json option optimizerSteps (in settings.optimizer.yulDetails) which determines the sequence of yul optimizer steps to be run. The default is defined in OptimiserSettings::DefaultYulOptimiserSteps in libsolidity/interface/OptimiserSettings.h. The sequence is actually run in OptimiserSuite::run (libyul/optimiser/Suite.cpp).

You'll see in OptimiserSuite::run, however, that we also have hardcoded parts. The initial sequence "hgfo" is fixed and cannot be changed, since it creates the standard form of Yul that's a prerequisite for all optimizer steps. However, for the "cleanup" sequence "fDnTOc g" (line 142), it makes sense to make it configurable as well.

So the task is to add a separate option, resp. a way to change, that cleanup sequence.

First thing to do would be to add a field for that in OptimiserSettings with the appropriate default. Then we'll need to decide how to set them from the outside in the CLI and standard json interfaces (I'm sceptical about a full new CLI option for it and am considering introducing a separator in the current option to split off the cleanup sequence, but I'd like @cameel's input on that).

There's one part of the cleanup sequence that will have to remain hardcoded, that's the "g" in the end. That's a hard requirement for the later pipeline.

[ekpyron]

So yeah, after talking with @cameel once more passing it in from CLI and standard json should work as follows:

We allow a colon in the optimizer steps string as a separator. The behaviour should be as follows:

• Without a colon, the sequence only sets the main optimizer steps just as it does now and uses the default "fDnTOc" as cleanup sequence.
• With a colon, everything before the colon is the main optimizer steps and everything after the colon replaces the cleanup sequence.

[cameel]

And we should also have a change in the breaking branch that disallows usage without a colon.

[nikola-matic]

@cameel, as discussed with @ekpyron, one of the things we'll disallow is custom cleanup sequence in nested sequences, i.e. a sequence such as dhfo[Dg:vu]lfnTUtnIf would be illegal, as we only want to see the : delimiter at nesting level zero. However, should we allow the delimiter to be placed at the beginning, or at the end of a sequence, thus making either the optimizer sequence or cleanup sequence empty? E.g., would these be OK: :dhfoDgvulfnTUtnIf or dhfoDgvulfnTUtnIf:?

[ekpyron]

Yes, both at the beginning and at the end is fine.

[ekpyron]

Also ":" is fine. That's actually among the expected use cases of this :-). (I.e. not perform any optimization other than stack compression and moving variables to memory, if running out of stack space - that'd be the effect of ":").

[cameel]

I just noticed that we did not update the docs for StandardJSON to mention the cleanup sequence: #13618.