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

Do not optimize IR without optimized outputs #15162

Merged
merged 2 commits into from
Jul 24, 2024

Conversation

cameel
Copy link
Member

@cameel cameel commented May 29, 2024

Fixes #15062.

@cameel cameel self-assigned this May 29, 2024
Copy link
Member Author

@cameel cameel May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that these tests do not really test much. I added them so that this unintuitive option combination at least gets used, but the change is not really visible in compiler output. We won't detect anything unless it just crashes.

The difference is only visible in execution time. Both of these should now finish almost instantly:

time solc test/benchmarks/chains.sol --ir --optimize
time solc test/benchmarks/chains.sol --ir --optimize --via-ir

@@ -0,0 +1 @@
--ir --optimize --debug-info none
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I have another one with --ir-optimized --optimize --debug-info none, so that we can at least see the difference between outputs in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. Only for this case though because I don't think such a test is overly useful. --ir-optimized --optimize is the standard thing to do and the test is not checking its correctness anyway.

@cameel cameel force-pushed the do-not-optimize-without-optimized-outputs branch 3 times, most recently from 49f1847 to 7565c52 Compare June 18, 2024 12:53
Copy link

github-actions bot commented Jul 3, 2024

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Jul 3, 2024
@cameel cameel removed the stale The issue/PR was marked as stale because it has been open for too long. label Jul 3, 2024
@cameel cameel force-pushed the do-not-optimize-without-optimized-outputs branch from 7565c52 to 5a232b8 Compare July 16, 2024 15:50
ekpyron
ekpyron previously approved these changes Jul 16, 2024
clonker
clonker previously approved these changes Jul 17, 2024
Copy link
Member

@clonker clonker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I was tripping over the _optimized argument in generateIR which also set to true if viaIR and generateEvmBytecode were active even if the goal wasn't set to Optimized until it occurred to me that eg stack allocations would still be optimized even if no optimize cli option was given. I think it might have been clearer to me if the argument was called _runOptimizer instead, which is also closer to the naming in the optimizer settings struct. Still, I think it's fine as it is.

@cameel cameel dismissed stale reviews from clonker and ekpyron via e40b184 July 18, 2024 13:41
@cameel cameel force-pushed the do-not-optimize-without-optimized-outputs branch from 5a232b8 to e40b184 Compare July 18, 2024 13:41
@cameel
Copy link
Member Author

cameel commented Jul 18, 2024

@clonker I think that the confusing thing is actually that the "optimized IR" output does not necessarily contain optimized IR :) It depends on the optimizer settings. It's just the main IR output that's used for compilation via IR and needing it for bytecode generation is natural, regardless of the optimization level. This is tripping people up all the time, especially on the CLI (@aarlt recently complained about that :)).

But I do think that things could be clearer. I wasn't fully satisfied with the initial version. I tried something new now - I replaced IRGenerationGoal with IROutputSelection, which should make it more apparent that it's really about outputs and not whether the optimizer runs or not. Let me know if you find that better.

clonker
clonker previously approved these changes Jul 19, 2024
Copy link
Member

@clonker clonker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You hit the nail on the head. That was the root of my confusion:) I like this version much better.

@cameel cameel force-pushed the do-not-optimize-without-optimized-outputs branch from e40b184 to 79e2bfc Compare July 19, 2024 12:49
@cameel
Copy link
Member Author

cameel commented Jul 19, 2024

Rebased to resolve a conflict with #15228.

I also tweaked it a little more for readability:

  • Renamed enableIRGeneration() to requestIROutputs() and adjusted its docstring. The new name should not suggest that it is the only way to enable IR generation.
  • Introduced intermediate booleans and some comments for the condition in CompilerStack::compile(). Hopefully it makes it even easier to see at a glance what's happening there since the nitty-gritty details are encapsulated and when looking at a condition the name already tells you all you need to know.
  • There were a few places with leftover goal naming (it's now called selection).

@cameel cameel force-pushed the do-not-optimize-without-optimized-outputs branch from 79e2bfc to 1a60d18 Compare July 24, 2024 15:59
@cameel cameel requested a review from clonker July 24, 2024 15:59
@cameel cameel merged commit b4c395b into develop Jul 24, 2024
72 checks passed
@cameel cameel deleted the do-not-optimize-without-optimized-outputs branch July 24, 2024 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--optimize should not trigger compilation if no optimized output is requested
4 participants