Skip to content
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

Enable minimal yul optimizations by default. #13972

Closed
ekpyron opened this issue Feb 13, 2023 · 8 comments · Fixed by #14149
Closed

Enable minimal yul optimizations by default. #13972

ekpyron opened this issue Feb 13, 2023 · 8 comments · Fixed by #14149
Assignees
Labels
high impact Changes are very prominent and affect users or the project in a major way. low effort There is not much implementation work to be done. The task is very easy or tiny. selected for development It's on our short-term development
Milestone

Comments

@ekpyron
Copy link
Member

ekpyron commented Feb 13, 2023

This was mentioned in other issues, including #13721 and #12533.

The gist is that we want to enable a minimal set of optimizations for the via-IR pipeline, such that:

  • Most contracts are likely to compile without error (stack-too-deep)
  • Tooling is still likely to be able to analyze bytecode without major hickups (e.g. conservative code deduplication, etc.), while also not having to deal with weird artifacts like redundant library references.
@ekpyron ekpyron added selected for development It's on our short-term development low effort There is not much implementation work to be done. The task is very easy or tiny. high impact Changes are very prominent and affect users or the project in a major way. labels Feb 13, 2023
@ekpyron ekpyron added this to the 0.8.20 milestone Feb 14, 2023
@cameel
Copy link
Member

cameel commented Apr 3, 2023

When we do this, we should replace the "unoptimized" preset and also the one with EVMASM optimizer only in external tests with one that uses this minimal optimization level. This will finally let us have all presets pass via IR.

@cameel
Copy link
Member

cameel commented Apr 12, 2023

Some extra details on the implementation from @ekpyron:

Basically a minimal version of the yul optimizer (like "u" and implicitly the optimized evm code transform and stack-to-memory) should run by default (i.e. with --via-ir without --optimize). But not the libevmasm optimizer or more yul optimization steps. And we need the additional option to disable those minimal optimizations as well.

Reconciling that with the existing options in a natural way (i.e. what happens when user explicitly sets settings.optimizer.yul to false?) is a part of the task. Best if we can limit ourselves to keeping existing options and just flipping their defaults.

@cameel
Copy link
Member

cameel commented Apr 17, 2023

@ekpyron I think that the simplest way to go about it is:

  1. Do not allow disabling Yul optimizer on the CLI.
  2. Change default optimizer sequence to "u" when --optimize is not provided
    • Not sure if we should let the user change it in that state. The optimizer is officially disabled after all.
  3. Change settings.optimizer.details.yul so that false does not disable Yul optimizer.
  4. settings.optimizer.details.yulDetails.optimizerSteps defaults to "u" when details.yul is set to false.

It's consistent with how currently optimizer.enabled: false does not really disable the optimizer.

Alternative solution

Still, I don't like the fact that options do not really mean what they sound like so here's an alternative way to do this:

  1. Add --no-optimize CLI option equivalent to OptimiserSettings::none(), i.e. no optimization at all
  2. Add new CLI option and make it the default: --optimize-minimal
    • equivalent to OptimiserSettings::minimal() + minimal Yul optimization
      • Yul optimizer enabled, but sequence set to "u"
      • yulDetails.stackAllocation enabled
      • peephole optimizer enabled
      • jumpdest remover enabled
  3. Make settings.optimizer.enabled: false, actually disable all of the optimizer.
  4. Add settings.optimizer.baseLevel, which only changes the set of defaults we use for optimizer settings:
    • null: defaults equivalent to --no-optimize
    • "minimal" (default): defaults equivalent to --optimize-minimal
    • "standard": defaults equivalent to --optimize
    • These levels can be now combined with existing settings if the user wants to override something.

This provides more fine grained control over the optimizer and makes the options actually mean what they sound like.

I suspect that you'll say this is too complicated, so I'm going to go with the first variant anyway, but I wanted to put it out there in case you do think this is worthwhile. There's a possibility that we actually do want to have a way to disable the Yul optimizer and the first variant makes it impossible.

@ekpyron
Copy link
Member Author

ekpyron commented Apr 17, 2023

We shouldn't care about CLI too much. We already can't e.g. disable the peephole optimizer in CLI - it's perfectly fine not to be able to disable minimal yul optimizations via CLI either. (And remember, we'll fix this with our CLI rewrite eventually ;-)).

Also to help you with disliking that the options don't do what they say they do:

With this change, you can actually consider the "minimal yul optimizations" as having the optimizer disabled. Those minimal optimizations just become parts of a Yul transformation for enabling code generation. That's really how we should think about this generally.
So it's perfectly fine to think of optimizations as disabled while still running those transformations.

So I'd go for the simplest solution here. Sounds good enough to me.

@fvictorio
Copy link
Contributor

Can someone summarize that this means for the next version? Specifically, what will be the behavior when you just have:

settings: {
  viaIR: true
}

and when you have:

settings: {
  optimizer: {
    enabled: true
  },
  viaIR: true
}

@cameel
Copy link
Member

cameel commented Jul 10, 2023

@fvictorio settings: {viaIR: true} is now equivalent to this:

settings: {
    viaIR: true,
    optimizer: {
        enabled: false,
        details: {
            yul: true,
            yulDetails: {
                stackAllocation: true,
                optimizerSteps: "u",
            }
        }
    }
}

but only as long as your contract does not contain (or call internally) any code that uses msize(). If it does, we fall back to the old behavior of yul: false (which can no longer be enabled explicitly by the user). This is per-contract, so one contract using msize() does not affect others that don't, but are also a part of compiler's input.

This applies to pure Yul compilation (i.e. language: "Yul") as well.

Behavior of settings: {viaIR: true, optimizer: {enabled: true}} did not change. I.e. you get both optimizers enabled, default optimization sequence and stackAllocation enabled.

@zerosnacks
Copy link

zerosnacks commented Jan 23, 2025

Hi @cameel

In Foundry we have a long standing issue foundry-rs/foundry#3357 with forge coverage where even with the recommended settings you mentioned above (which we refer to as --ir-minimum) our users are running into issues of unreliable source mappings (and stack too deep) making it impossible to construct accurate reports. Do you see any solutions the Solidity compiler could offer here? Having accurate coverage reports is critical in the process of developing secure smart contracts.

Thanks!

@cameel
Copy link
Member

cameel commented Jan 27, 2025

@zerosnacks Unfortunately I can't offer any quick fix. The limitation ultimately lies in the current implementation of our Yul->EVM transform. It's sometimes unable to produce a viable stack layout and gives up. The minimum optimizations added here (i.e. unused pruner) reduce the stack pressure, making this happen less but there are still some hard cases where this happens even with the help of the stack-to-memory mover.

This will be solved eventually, but it'll still be a while until any of the solutions land. On one hand we're working on a better Yul->EVM transform. On other the EVM is also moving towards EOF where the problem will no longer exist thanks to SWAPN/DUPN opcodes.

The prototype of the EOF backend will actually ship in the next release and relaxing stack constraints is the very next EOF feature that will be worked on after that. EOF support will be considered experimental until EOF hits mainnet and it would not be running the exact same bytecode, but the difference might not matter in practice, especially if the user is aware of the difference (i.e. has to explicitly enable it via a flag) so maybe it would be usable for coverage calculation. The biggest problem here will probably be that there will be syntax differences so the code may not run as is. We're still considering making it possible to write portable code though. Feedback welcome: [EOF] In-language construct to check for EOF.

One thing we might do right now could be to make it a bit easier for users to find out what is triggering StackTooDeep and work around it (#12449). The problem usually does not sit in anything fundamental and is pretty ephemeral. Just restructuring code around it in various ways often makes it go away. The problem is just that finding the place to point at is mostly guesswork. The issue is non-local, so the point where it happened is sometimes not the place that's really causing it. But it still could be better than nothing and I think it should be provided at least as a hint.

users are running into issues of unreliable source mappings

Regarding source mappings - are they wrong or just incomplete? If it's the former, please report as a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high impact Changes are very prominent and affect users or the project in a major way. low effort There is not much implementation work to be done. The task is very easy or tiny. selected for development It's on our short-term development
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants
@cameel @fvictorio @ekpyron @zerosnacks and others