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

Accept empty optimizer sequence with Yul optimizer disabled #14657

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ Language Features:


Compiler Features:

* Commandline Interface: An empty ``--yul-optimizations`` sequence can now be always provided.
* Standard JSON Interface: An empty ``optimizerSteps`` sequence can now always be provided.

Bugfixes:

Expand Down
6 changes: 6 additions & 0 deletions docs/internals/optimizer.rst
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ for a stand-alone Yul mode.
The `peephole optimizer <https://en.wikipedia.org/wiki/Peephole_optimization>`_ is always
enabled by default and can only be turned off via the :ref:`Standard JSON <compiler-api>`.

.. note::
An empty optimizer sequence is accepted even without ``--optimize`` in order to fully disable
the user-supplied portion of the Yul :ref:`optimizer sequence <selecting-optimizations>`, as by default,
even when the optimizer is not turned on, the :ref:`unused pruner <unused-pruner>` step will be run.

You can find more details on both optimizer modules and their optimization steps below.

Benefits of Optimizing Solidity Code
Expand Down Expand Up @@ -329,6 +334,7 @@ Abbreviation Full name
Some steps depend on properties ensured by ``BlockFlattener``, ``FunctionGrouper``, ``ForLoopInitRewriter``.
For this reason the Yul optimizer always applies them before applying any steps supplied by the user.

.. _selecting-optimizations:

Selecting Optimizations
-----------------------
Expand Down
17 changes: 17 additions & 0 deletions libsolidity/interface/CompilerStack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
#include <libyul/YulStack.h>
#include <libyul/AST.h>
#include <libyul/AsmParser.h>
#include <libyul/optimiser/Suite.h>

#include <liblangutil/Scanner.h>
#include <liblangutil/SemVerHandler.h>
Expand Down Expand Up @@ -99,6 +100,7 @@ using namespace solidity;
using namespace solidity::langutil;
using namespace solidity::frontend;
using namespace solidity::stdlib;
using namespace solidity::yul;
using namespace std::string_literals;

using solidity::util::errinfo_comment;
Expand Down Expand Up @@ -1672,6 +1674,21 @@ std::string CompilerStack::createMetadata(Contract const& _contract, bool _forIR
details["yulDetails"]["stackAllocation"] = m_optimiserSettings.optimizeStackAllocation;
details["yulDetails"]["optimizerSteps"] = m_optimiserSettings.yulOptimiserSteps + ":" + m_optimiserSettings.yulOptimiserCleanupSteps;
}
else if (
OptimiserSuite::isEmptyOptimizerSequence(m_optimiserSettings.yulOptimiserSteps) &&
OptimiserSuite::isEmptyOptimizerSequence(m_optimiserSettings.yulOptimiserCleanupSteps)
)
{
solAssert(m_optimiserSettings.optimizeStackAllocation == false);
details["yulDetails"] = Json::objectValue;
details["yulDetails"]["optimizerSteps"] = ":";
}
else
{
solAssert(m_optimiserSettings.optimizeStackAllocation == false);
solAssert(m_optimiserSettings.yulOptimiserSteps == OptimiserSettings::DefaultYulOptimiserSteps);
solAssert(m_optimiserSettings.yulOptimiserCleanupSteps == OptimiserSettings::DefaultYulOptimiserCleanupSteps);
}

meta["settings"]["optimizer"]["details"] = std::move(details);
}
Expand Down
20 changes: 14 additions & 6 deletions libsolidity/interface/StandardCompiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -456,12 +456,16 @@ std::optional<Json::Value> checkOptimizerDetail(Json::Value const& _details, std
return {};
}

std::optional<Json::Value> checkOptimizerDetailSteps(Json::Value const& _details, std::string const& _name, std::string& _optimiserSetting, std::string& _cleanupSetting)
std::optional<Json::Value> checkOptimizerDetailSteps(Json::Value const& _details, std::string const& _name, std::string& _optimiserSetting, std::string& _cleanupSetting, bool _runYulOptimizer)
{
if (_details.isMember(_name))
{
if (_details[_name].isString())
{
std::string const fullSequence = _details[_name].asString();
if (!_runYulOptimizer && !OptimiserSuite::isEmptyOptimizerSequence(fullSequence))
return formatFatalError(Error::Type::JSONError, "If Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted.");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return formatFatalError(Error::Type::JSONError, "If Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted.");
return formatFatalError(Error::Type::JSONError, "When Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted.");


try
{
yul::OptimiserSuite::validateSequence(_details[_name].asString());
Expand All @@ -474,7 +478,6 @@ std::optional<Json::Value> checkOptimizerDetailSteps(Json::Value const& _details
);
}

std::string const fullSequence = _details[_name].asString();
auto const delimiterPos = fullSequence.find(":");
_optimiserSetting = fullSequence.substr(0, delimiterPos);

Expand Down Expand Up @@ -605,22 +608,27 @@ std::variant<OptimiserSettings, Json::Value> parseOptimizerSettings(Json::Value
if (details.isMember("yulDetails"))
{
if (!settings.runYulOptimiser)
return formatFatalError(Error::Type::JSONError, "\"Providing yulDetails requires Yul optimizer to be enabled.");
{
if (checkKeys(details["yulDetails"], {"optimizerSteps"}, "settings.optimizer.details.yulDetails"))
return formatFatalError(Error::Type::JSONError, "Only optimizerSteps can be set in yulDetails when Yul optimizer is disabled.");
r0qs marked this conversation as resolved.
Show resolved Hide resolved
if (auto error = checkOptimizerDetailSteps(details["yulDetails"], "optimizerSteps", settings.yulOptimiserSteps, settings.yulOptimiserCleanupSteps, settings.runYulOptimiser))
return *error;
return {std::move(settings)};
}

if (auto result = checkKeys(details["yulDetails"], {"stackAllocation", "optimizerSteps"}, "settings.optimizer.details.yulDetails"))
return *result;
if (auto error = checkOptimizerDetail(details["yulDetails"], "stackAllocation", settings.optimizeStackAllocation))
return *error;
if (auto error = checkOptimizerDetailSteps(details["yulDetails"], "optimizerSteps", settings.yulOptimiserSteps, settings.yulOptimiserCleanupSteps))
if (auto error = checkOptimizerDetailSteps(details["yulDetails"], "optimizerSteps", settings.yulOptimiserSteps, settings.yulOptimiserCleanupSteps, settings.runYulOptimiser))
return *error;
}
}
return { std::move(settings) };
return {std::move(settings)};
}

}


std::variant<StandardCompiler::InputsAndSettings, Json::Value> StandardCompiler::parseInput(Json::Value const& _input)
{
InputsAndSettings ret;
Expand Down
33 changes: 30 additions & 3 deletions libyul/YulStack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,14 @@
#include <libyul/optimiser/Suite.h>
#include <libevmasm/Assembly.h>
#include <liblangutil/Scanner.h>
#include <libsolidity/interface/OptimiserSettings.h>

#include <boost/algorithm/string.hpp>

#include <optional>

using namespace solidity;
using namespace solidity::frontend;
using namespace solidity::yul;
using namespace solidity::langutil;

Expand Down Expand Up @@ -164,14 +166,39 @@ void YulStack::optimize(Object& _object, bool _isCreation)
if (EVMDialect const* evmDialect = dynamic_cast<EVMDialect const*>(&dialect))
meter = std::make_unique<GasMeter>(*evmDialect, _isCreation, m_optimiserSettings.expectedExecutionsPerDeployment);

auto [optimizeStackAllocation, yulOptimiserSteps, yulOptimiserCleanupSteps] = [&]() -> std::tuple<bool, std::string, std::string>
{
if (!m_optimiserSettings.runYulOptimiser)
{
// Yul optimizer disabled, but empty sequence (:) explicitly provided
if (OptimiserSuite::isEmptyOptimizerSequence(m_optimiserSettings.yulOptimiserSteps + ":" + m_optimiserSettings.yulOptimiserCleanupSteps))
nikola-matic marked this conversation as resolved.
Show resolved Hide resolved
cameel marked this conversation as resolved.
Show resolved Hide resolved
return std::make_tuple(true, "", "");
// Yul optimizer disabled, and no sequence explicitly provided (assumes default sequence)
nikola-matic marked this conversation as resolved.
Show resolved Hide resolved
else
{
yulAssert(
m_optimiserSettings.yulOptimiserSteps == OptimiserSettings::DefaultYulOptimiserSteps &&
m_optimiserSettings.yulOptimiserCleanupSteps == OptimiserSettings::DefaultYulOptimiserCleanupSteps
);
return std::make_tuple(true, "u", "");
}

}
return std::make_tuple(
m_optimiserSettings.optimizeStackAllocation,
m_optimiserSettings.yulOptimiserSteps,
m_optimiserSettings.yulOptimiserCleanupSteps
);
}();

OptimiserSuite::run(
dialect,
meter.get(),
_object,
// Defaults are the minimum necessary to avoid running into "Stack too deep" constantly.
m_optimiserSettings.runYulOptimiser ? m_optimiserSettings.optimizeStackAllocation : true,
m_optimiserSettings.runYulOptimiser ? m_optimiserSettings.yulOptimiserSteps : "u",
m_optimiserSettings.runYulOptimiser ? m_optimiserSettings.yulOptimiserCleanupSteps : "",
optimizeStackAllocation,
yulOptimiserSteps,
yulOptimiserCleanupSteps,
_isCreation ? std::nullopt : std::make_optional(m_optimiserSettings.expectedExecutionsPerDeployment),
{}
);
Expand Down
19 changes: 19 additions & 0 deletions libyul/optimiser/Suite.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,25 @@ void OptimiserSuite::validateSequence(std::string_view _stepAbbreviations)
assertThrow(nestingLevel == 0, OptimizerException, "Unbalanced brackets");
}

bool OptimiserSuite::isEmptyOptimizerSequence(std::string const& _sequence)
nikola-matic marked this conversation as resolved.
Show resolved Hide resolved
{
size_t delimiterCount{0};
for (char const step: _sequence)
switch (step)
{
case ':':
if (++delimiterCount > 1)
return false;
break;
nikola-matic marked this conversation as resolved.
Show resolved Hide resolved
case ' ':
case '\n':
break;
default:
return false;
}
return true;
}
Comment on lines +387 to +403
Copy link
Member

@ekpyron ekpyron Nov 6, 2023

Choose a reason for hiding this comment

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

I mean, I'm very much fine with leaving this as is - but given that I think I even already saw some discussion in the channel around this on the side, I can't help but wonder: why the special handling of different counts of colons in here and all the fuzz (the sequence will separately be validated and arguably it even makes more sense to treat "::" as empty but malformed rather than as non-empty...)?

This could just be (inline):

Suggested change
{
size_t delimiterCount{0};
for (char const step: _sequence)
switch (step)
{
case ':':
if (++delimiterCount > 1)
return false;
break;
case ' ':
case '\n':
break;
default:
return false;
}
return true;
}
{
return ranges::any_of(_sequence, [](auto _step) { return _step != ':' && _step != ' ' && _step != '\n'; });
}

in one case - and in the second case for standard json even simpler...


void OptimiserSuite::runSequence(std::string_view _stepAbbreviations, Block& _ast, bool _repeatUntilStable)
{
validateSequence(_stepAbbreviations);
Expand Down
4 changes: 4 additions & 0 deletions libyul/optimiser/Suite.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ class OptimiserSuite
/// Ensures that specified sequence of step abbreviations is well-formed and can be executed.
/// @throw OptimizerException if the sequence is invalid
static void validateSequence(std::string_view _stepAbbreviations);
/// Check whether the provided sequence is empty provided that the allowed characters are
/// whitespace, newline and :
static bool isEmptyOptimizerSequence(std::string const& _sequence);


void runSequence(std::vector<std::string> const& _steps, Block& _ast);
void runSequence(std::string_view _stepAbbreviations, Block& _ast, bool _repeatUntilStable = false);
Expand Down
5 changes: 3 additions & 2 deletions solc/CommandLineParser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include <fmt/format.h>

using namespace solidity::langutil;
using namespace solidity::yul;

namespace po = boost::program_options;

Expand Down Expand Up @@ -1228,8 +1229,8 @@ void CommandLineParser::processArgs()
if (m_args.count(g_strYulOptimizations))
{
OptimiserSettings optimiserSettings = m_options.optimiserSettings();
if (!optimiserSettings.runYulOptimiser)
solThrow(CommandLineValidationError, "--" + g_strYulOptimizations + " is invalid if Yul optimizer is disabled");
if (!optimiserSettings.runYulOptimiser && !OptimiserSuite::isEmptyOptimizerSequence(m_args[g_strYulOptimizations].as<std::string>()))
solThrow(CommandLineValidationError, "--" + g_strYulOptimizations + " is invalid with a non-empty sequence if Yul optimizer is disabled.");

try
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity >=0.0;

contract C {
function f() public pure {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"language": "Solidity",
"sources": {
"A": {"urls": ["standard_optimizer_yulDetails_optimiserSteps_no_yul/in.sol"]}
},
"settings": {
"optimizer": {
"details": {
"yul": false,
"yulDetails": {
"optimizerSteps": "u:fdntOc"
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"errors":
[
{
"component": "general",
"formattedMessage": "If Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted.",
"message": "If Yul optimizer is disabled, only an empty optimizerSteps sequence is accepted.",
"severity": "error",
"type": "JSONError"
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity >=0.0;

contract C {
function f() public pure {}
}
nikola-matic marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"language": "Solidity",
"sources": {
"A": {"urls": ["standard_optimizer_yulDetails_optimiserSteps_with_empty_sequence_no_yul/in.sol"]}
},
"settings": {
"optimizer": {
"details": {
"yul": false,
"yulDetails": {
"optimizerSteps": ":"
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"sources":
{
"A":
{
"id": 0
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity >=0.0;

contract C {
function f() public pure {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"language": "Solidity",
"sources": {
"A": {"urls": ["standard_optimizer_yulDetails_optimiserSteps_with_whitespace_newline_sequence_no_yul/in.sol"]}
},
"settings": {
"optimizer": {
"details": {
"yul": false,
"yulDetails": {
"optimizerSteps": "\n : "
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"sources":
{
"A":
{
"id": 0
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
[
{
"component": "general",
"formattedMessage": "\"Providing yulDetails requires Yul optimizer to be enabled.",
"message": "\"Providing yulDetails requires Yul optimizer to be enabled.",
"formattedMessage": "Only optimizerSteps can be set in yulDetails when Yul optimizer is disabled.",
"message": "Only optimizerSteps can be set in yulDetails when Yul optimizer is disabled.",
"severity": "error",
"type": "JSONError"
}
Expand Down
2 changes: 1 addition & 1 deletion test/cmdlineTests/yul_optimizer_steps_disabled/err
Original file line number Diff line number Diff line change
@@ -1 +1 @@
Error: --yul-optimizations is invalid if Yul optimizer is disabled
Error: --yul-optimizations is invalid with a non-empty sequence if Yul optimizer is disabled.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
--ir-optimized --metadata --yul-optimizations :
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
Warning: Unused local variable.
--> yul_optimizer_steps_without_optimize_empty_sequence/input.sol:13:9:
|
13 | uint b = a;
| ^^^^^^

Warning: Unused local variable.
--> yul_optimizer_steps_without_optimize_empty_sequence/input.sol:14:9:
|
14 | uint c = a;
| ^^^^^^
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity >=0.0;

contract C
{
constructor() {}

function foo() public pure returns (bool)
{
// given the empty optimizer sequence ``:``, ``b`` and ``c`` should not be removed in the
// optimized IR as the ``UnusedPruner`` step will not be run.
uint a = 100;
uint b = a;
uint c = a;
nikola-matic marked this conversation as resolved.
Show resolved Hide resolved

return a == 100;
}
}
Loading