Introduce global optimiser settings. #5959

chriseth · 2019-02-07T17:04:25Z

Replaces PR #2600
References issue #1658

Apart from exporting the settings via the metadata, this should be a pure refactoring.

Still todo:

enable the yul optimiser based on the settings
parse the settings from standard-json-io
parse the settings from the commandline (do we really want 100% control from the CLI?)

chriseth · 2019-02-07T17:09:45Z

If we agree on the metadata strings, we can already merge it like this.

libsolidity/interface/CompilerStack.cpp

codecov · 2019-02-07T19:51:39Z

Codecov Report

Merging #5959 into develop will increase coverage by <.01%.
The diff coverage is 87.82%.

@@             Coverage Diff             @@
##           develop    #5959      +/-   ##
===========================================
+ Coverage    87.91%   87.92%   +<.01%     
===========================================
  Files          374      375       +1     
  Lines        35771    35973     +202     
  Branches      4220     4245      +25     
===========================================
+ Hits         31447    31628     +181     
- Misses        2901     2910       +9     
- Partials      1423     1435      +12

Flag	Coverage Δ
#all	`87.92% <87.82%> (ø)`	⬆️
#syntax	`27.17% <3.35%> (-0.14%)`	⬇️

chriseth · 2019-02-07T19:52:20Z

Compilation error for emscripten:

/root/project/libsolidity/interface/CompilerStack.cpp:934:64: error: result of comparison of constant 18446744073709551615 with expression of type 'const size_t' (aka 'const unsigned long') is always true [-Werror,-Wtautological-constant-out-of-range-compare]
        solAssert(m_optimiserSettings.expectedExecutionsPerDeployment < std::numeric_limits<Json::LargestUInt>::max(), "");
                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/root/project/liblangutil/Exceptions.h:45:21: note: expanded from macro 'solAssert'

I believe we already had something like that in the past. Anyone remembers how we solved it?

ekpyron · 2019-02-11T14:05:05Z

@chriseth The emscripten compile error is due to the emscripten build having 32-bit size_t - so to avoid the assertion from being trivial due to type constraints, we need to cast, but we have to be careful not to defeat the purpose of the assertion by doing so (due to potentially truncating during the cast...)...
The following would probably do it, but is not particularly nice:

solAssert(u256(m_optimiserSettings.expectedExecutionsPerDeployment) < u256(std::numeric_limits<Json::LargestUInt>::max()), "")

C++17 would be nice here, then we could do

if constexpr (sizeof(m_optimiserSettings.expectedExecutionsPerDeployment) >= sizeof(Json::LargestUInt))
    solAssert(m_optimiserSettings.expectedExecutionsPerDeployment < std::numeric_limits<Json::LargestUInt>::max(), "");

Maybe there's a nicer solution, but that's the problem in any case.

EDIT:
Maybe the following is best:

static_assert(sizeof(m_optimiserSettings.expectedExecutionsPerDeployment) <= sizeof(JSon::LargestUint));
solAssert(static_cast<JSon::LargestUint>(m_optimiserSettings.expectedExecutionsPerDeployment) < std::numeric_limits<Json::LargestUInt>::max(), "")

docs/metadata.rst

axic · 2019-02-19T17:33:16Z

I suggest the following:

If any fine-grained setting is provided in standard json, then the enabled option cannot be present.
If the fine-grained settings equal to that of the default enabled: false or enabled: true, then the metadata only contains the enabled field, but not the fine-grained settings. Otherwise only the fine-grained settings are stored.

In 0.6.0 we may revisit dropping enabled.

axic · 2019-02-19T17:55:06Z

From off-line discussion: it seems that introducing a new sub-level for optimiser "details" is a better approach.

Example:

    // Optional: Optimizer settings. The fields "enabled" and "runs" are deprecated
    // and are only given for backwards-compatibility.
    optimizer: {
      enabled: true,
      runs: 200,
      details: {
          // peephole defaults to "true"
          peephole: true,
          // jumpdestRemover defaults to "true"
          jumpdestRemover: true,
          orderLiterals: false,
          deduplicate: false,
          cse: false,
          constantOptimizer: false,
          yul: true
          yulDetails: {}
      },
    },

Subtleties:

if the enabled field is set to false, optimisations are entirely disabled and the metadata will not contain the details object.
if the enabled field is set to true, optimisations are enabled with a default preset. The details object can be present and can modify these settings. If the details section changes any of the settings from the default present, the entire object is included in the metadata.

The runs parameter is still preferred to be renamed to expectedExecutionsPerDeployment, but that seems to be a breaking change. Also don't forget that as of late, this field is used outside of the optimiser to determine the optimal layout of the "ABI dispatcher code", therefore it is suggested to be moved outside of the optimizer object. This should be done in a separate change together with renaming the field.

chriseth · 2019-02-21T16:30:56Z

If "details" is not a member, optimizer will be in "default disabled" mode, which mean peephole and jumpdest remover are still on.

chriseth · 2019-02-21T17:32:15Z

@axic should we also add a "stackAllocation" option to the optimizer? This would then activate the _allowStackOpt switch of CodeTransform.

chriseth · 2019-02-21T17:34:25Z

Hm, that could just be the yul optimizer option.

libsolidity/codegen/Compiler.cpp

chriseth · 2019-02-25T14:09:11Z

Updated.

libsolidity/codegen/ExpressionCompiler.cpp

axic · 2019-03-01T01:29:22Z

All of the gas tests are broken and every single one becomes more expensive. Any idea why? This PR shouldn't change behaviour of the optimiser.

chriseth · 2019-03-01T12:25:54Z

test/libsolidity/SolidityExpressionCompiler.cpp

@@ -153,7 +153,7 @@ bytes compileFirstExpression(
 					parametersSize--
 				);

-			ExpressionCompiler(context).compile(*extractor.expression());
+			ExpressionCompiler(context, dev::test::Options::get().optimize).compile(*extractor.expression());


The reason for these tests to change was that we did not take the global test parameter into account before. I think this is a good test, because it shows the effect of order literals optimization.

But all the deployment sizes increased.

No, the non-optimized expectations are unchanged.

chriseth · 2019-03-01T12:26:39Z

test/libsolidity/Assembly.cpp

-			Compiler compiler(dev::test::Options::get().evmVersion());
+			Compiler compiler(
+				dev::test::Options::get().evmVersion(),
+				dev::test::Options::get().optimize ? OptimiserSettings::enabled() : OptimiserSettings::minimal()


Same here, optimizer setting was ignored before.

libsolidity/codegen/ContractCompiler.h

chriseth · 2019-03-01T13:14:13Z

I also don't understand why the commandline tests changed - will have to investigate. It might be that "stack allocation" was activated before when the optimizer was activated, now it can only be activated manually.

chriseth · 2019-03-04T10:45:38Z

Ok, the problem in the commandline tests was actually related to the invalid reference noted above.

axic

On the assumption that apart from the const& change this is identical to last week.

chriseth · 2019-03-04T11:54:16Z

Yep, removed the reference and also removed the test expectation changes the reference caused.

chriseth force-pushed the optimiser2 branch from 746af1c to 2cd98db Compare February 7, 2019 17:09

chriseth commented Feb 7, 2019

View reviewed changes

libsolidity/interface/CompilerStack.cpp Outdated Show resolved Hide resolved

chriseth commented Feb 7, 2019

View reviewed changes

libsolidity/interface/CompilerStack.cpp Outdated Show resolved Hide resolved

chriseth force-pushed the optimiser2 branch 2 times, most recently from d5e72d0 to 1a34e66 Compare February 7, 2019 19:51

chriseth force-pushed the optimiser2 branch 3 times, most recently from 322a657 to e2eb48a Compare February 11, 2019 16:06

chriseth changed the title ~~Introduce global optimiser settings.~~ [WIP] Introduce global optimiser settings. Feb 11, 2019

axic reviewed Feb 19, 2019

View reviewed changes

docs/metadata.rst Show resolved Hide resolved

chriseth force-pushed the optimiser2 branch from e2eb48a to 86975c7 Compare February 21, 2019 12:43

chriseth mentioned this pull request Feb 21, 2019

[WIP] Enable yul optimizer #5954

Closed

chriseth force-pushed the optimiser2 branch from 0a3ca92 to 260dfc2 Compare February 21, 2019 17:48

chriseth changed the title ~~[WIP] Introduce global optimiser settings.~~ Introduce global optimiser settings. Feb 21, 2019

chriseth force-pushed the optimiser2 branch from e4621ac to a3804be Compare February 21, 2019 18:02

axic reviewed Feb 25, 2019

View reviewed changes

libsolidity/codegen/Compiler.cpp Show resolved Hide resolved

chriseth force-pushed the optimiser2 branch from a3804be to 2dadf4c Compare February 25, 2019 14:08

axic reviewed Feb 25, 2019

View reviewed changes

libsolidity/codegen/ExpressionCompiler.cpp Show resolved Hide resolved

chriseth mentioned this pull request Feb 25, 2019

[WIP] Implement fine-grained control over the optimiser #2600

Closed

4 tasks

chriseth force-pushed the optimiser2 branch from 2dadf4c to 9222abb Compare February 26, 2019 11:41

axic previously approved these changes Mar 1, 2019

View reviewed changes

axic dismissed their stale review via 4808f1b March 1, 2019 00:52

axic force-pushed the optimiser2 branch from fdf1f44 to 4808f1b Compare March 1, 2019 00:52

axic force-pushed the optimiser2 branch from 4808f1b to 102e79a Compare March 1, 2019 01:10

chriseth commented Mar 1, 2019

View reviewed changes

libsolidity/codegen/ContractCompiler.h Outdated Show resolved Hide resolved

axic and others added 7 commits March 4, 2019 11:40

Introduce global optimiser settings.

cf5c13f

Process and validate standard-json optimizer settings.

4d10f4b

Provide full optimiser settings to ContractCompiler.

1ff562d

Take yul optimizer setting into account.

83d1382

Provide commandline option to activate yul optimizer.

5ddbc43

Remove defaults in constructor arguments.

851dd63

Update tests.

43850d5

chriseth force-pushed the optimiser2 branch from 102e79a to 43850d5 Compare March 4, 2019 10:45

axic approved these changes Mar 4, 2019

View reviewed changes

chriseth merged commit 2e0ea16 into develop Mar 4, 2019

axic deleted the optimiser2 branch March 4, 2019 11:55

axic mentioned this pull request Mar 4, 2019

Metadata should not require compilation #5941

Merged

ekpyron mentioned this pull request Mar 6, 2019

Simplify constant switch first #6208

Merged

axic mentioned this pull request Mar 20, 2019

Fine grained control of the optimiser #1658

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce global optimiser settings. #5959

Introduce global optimiser settings. #5959

chriseth commented Feb 7, 2019 •

edited

Loading

chriseth commented Feb 7, 2019

codecov bot commented Feb 7, 2019 •

edited

Loading

chriseth commented Feb 7, 2019

ekpyron commented Feb 11, 2019 •

edited

Loading

axic commented Feb 19, 2019 •

edited

Loading

axic commented Feb 19, 2019

chriseth commented Feb 21, 2019

chriseth commented Feb 21, 2019

chriseth commented Feb 21, 2019

chriseth commented Feb 25, 2019

axic commented Mar 1, 2019

chriseth Mar 1, 2019

axic Mar 1, 2019

chriseth Mar 1, 2019

chriseth Mar 1, 2019

chriseth commented Mar 1, 2019

chriseth commented Mar 4, 2019

axic left a comment

chriseth commented Mar 4, 2019

Introduce global optimiser settings. #5959

Introduce global optimiser settings. #5959

Conversation

chriseth commented Feb 7, 2019 • edited Loading

chriseth commented Feb 7, 2019

codecov bot commented Feb 7, 2019 • edited Loading

Codecov Report

chriseth commented Feb 7, 2019

ekpyron commented Feb 11, 2019 • edited Loading

axic commented Feb 19, 2019 • edited Loading

axic commented Feb 19, 2019

chriseth commented Feb 21, 2019

chriseth commented Feb 21, 2019

chriseth commented Feb 21, 2019

chriseth commented Feb 25, 2019

axic commented Mar 1, 2019

chriseth Mar 1, 2019

Choose a reason for hiding this comment

axic Mar 1, 2019

Choose a reason for hiding this comment

chriseth Mar 1, 2019

Choose a reason for hiding this comment

chriseth Mar 1, 2019

Choose a reason for hiding this comment

chriseth commented Mar 1, 2019

chriseth commented Mar 4, 2019

axic left a comment

Choose a reason for hiding this comment

chriseth commented Mar 4, 2019

chriseth commented Feb 7, 2019 •

edited

Loading

codecov bot commented Feb 7, 2019 •

edited

Loading

ekpyron commented Feb 11, 2019 •

edited

Loading

axic commented Feb 19, 2019 •

edited

Loading