Skip to content

Conversation

@cameel
Copy link
Collaborator

@cameel cameel commented Aug 3, 2021

Related to #11720.
Also implements part of #11629.

There were multiple places that initialized OptimiserSettings: it's needed for validation in CommandLineParser and later in CommandLineInterface to actually initialize the compiler. In both cases the initialization was performed separately for the compiler and for the assembler. Now there's a single function that creates the struct based on CommandLineOptions.

This change would result in optimizer settings being parsed (but unused) in linker mode so I also made the compiler reject them in this mode and also in Standard JSON mode.

@cameel cameel self-assigned this Aug 3, 2021
@cameel cameel force-pushed the deduplicate-cli-optimizer-initialization branch from 753d8ef to 301415f Compare August 3, 2021 15:36

if (m_options.optimizer.yulSteps.has_value())
settings.yulOptimiserSteps = m_options.optimizer.yulSteps.value();
settings.optimizeStackAllocation = settings.runYulOptimiser;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that this line was not in assemble(), only in compile(). I removed it because it's redundant anyway. With current defaults in OptimierSettings the value of optimizeStackAllocation always matches runYulOptimiser. On the CLI they can only be different if you use --no-optimize-yul and that option is invalid in assembly mode.

@cameel cameel force-pushed the deduplicate-cli-optimizer-initialization branch from 301415f to 272f53b Compare August 17, 2021 10:51
@cameel cameel force-pushed the deduplicate-cli-optimizer-initialization branch from 272f53b to 84f66bd Compare August 31, 2021 15:34
ekpyron
ekpyron previously approved these changes Sep 10, 2021
Copy link
Collaborator

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Looks fine apart from maybe the changelog entry.

Although (I keep saying this), I still think it's a waste of time and energy meddling with this infernal command line interface instead of replacing it by a translation layer to StandardCompiler right away :-).

@cameel
Copy link
Collaborator Author

cameel commented Sep 10, 2021

Although (I keep saying this), I still think it's a waste of time and energy meddling with this infernal command line interface instead of replacing it by a translation layer to StandardCompiler right away :-).

But we won't be throwing out CommandLineParser completely, will we? It's completely CLI-specific and half of this change touches the code there. The other half deals with converting optimizer options to OptimiserSettings - that part would be gone but instead we'd have to convert them to Standard JSON input. So this refactor would make sense even after such a rewrite.

Also, I went though CommandLineInterface to see how much would be rewritten and a lot of it deals with formatting the CLI output. Input parsing was also a large part of it but that's now in CommandLineParser. The part that would actually have to be rewritten on top of Standard JSON is in just 3 functions: compile(), handleAst() and assemble(). That's ~250 lines out of more than 1000 (+ another 1000 in CommandLineParser). I think that extracting output formatting into a separate component, like I did with CommandLineParser would actually make more sense. Then we could rewrite or improve each part separately instead of having a monster task to rewrite everything.

@ekpyron
Copy link
Collaborator

ekpyron commented Sep 10, 2021

Although (I keep saying this), I still think it's a waste of time and energy meddling with this infernal command line interface instead of replacing it by a translation layer to StandardCompiler right away :-).

But we won't be throwing out CommandLineParser completely, will we? It's completely CLI-specific and half of this change touches the code there. The other half deals with converting optimizer options to OptimiserSettings - that part would be gone but instead we'd have to convert them to Standard JSON input. So this refactor would make sense even after such a rewrite.

Also, I went though CommandLineInterface to see how much would be rewritten and a lot of it deals with formatting the CLI output. Input parsing was also a large part of it but that's now in CommandLineParser. The part that would actually have to be rewritten on top of Standard JSON is in just 3 functions: compile(), handleAst() and assemble(). That's ~250 lines out of more than 1000 (+ another 1000 in CommandLineParser). I think that extracting output formatting into a separate component, like I did with CommandLineParser would actually make more sense. Then we could rewrite or improve each part separately instead of having a monster task to rewrite everything.

Well ok, fair enough - you may be right that that may in fact be the best way to achieve the same thing.

@cameel cameel force-pushed the deduplicate-cli-optimizer-initialization branch from 84f66bd to 9737356 Compare September 13, 2021 13:28
@cameel cameel force-pushed the deduplicate-cli-optimizer-initialization branch from 9737356 to 1e4cef8 Compare September 13, 2021 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants