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.

Sign up for GitHub

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

Jump to bottom

Prepare literalArguments for immutable builtin functions #8626

Merged
merged 1 commit into from Apr 9, 2020
Merged

Prepare literalArguments for immutable builtin functions #8626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 6 additions & 4 deletions libyul/AsmAnalysis.cpp
View file
Open in desktop
Expand Up @@ -255,14 +255,14 @@ vector<YulString> AsmAnalyzer::operator()(FunctionCall const& _funCall)
yulAssert(!_funCall.functionName.name.empty(), "");
vector<YulString> const* parameterTypes = nullptr;
vector<YulString> const* returnTypes = nullptr;
bool needsLiteralArguments = false;
vector<bool> const* needsLiteralArguments = nullptr;

if (BuiltinFunction const* f = m_dialect.builtin(_funCall.functionName.name))
{
parameterTypes = &f->parameters;
returnTypes = &f->returns;
if (f->literalArguments)
needsLiteralArguments = true;
needsLiteralArguments = &f->literalArguments.value();
}
else if (!m_currentScope->lookup(_funCall.functionName.name, GenericVisitor{
[&](Scope::Variable const&)
Expand Down Expand Up @@ -293,11 +293,13 @@ vector<YulString> AsmAnalyzer::operator()(FunctionCall const& _funCall)
);

vector<YulString> argTypes;
for (auto const& arg: _funCall.arguments | boost::adaptors::reversed)
for (size_t i = _funCall.arguments.size(); i > 0; i--)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we create a helper for that? I think the i - 1 is really dangerous. Is there something like python's enumerate? Then we could use for (auto&& [i, arg]: enumerate(_funCall.arguments) | reversed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes absolutely. I wasn't happy with that either.

Copy link
Contributor

Choose a reason for hiding this comment

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

http://reedbeta.com/blog/python-like-enumerate-in-cpp17/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@christianparpart do you have a good idea how this could be done?

Copy link
Member

@ekpyron ekpyron Apr 8, 2020 •
edited

Choose a reason for hiding this comment

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

Maybe just

for (size_t i : boost::counting_range(0, _funCall.arguments.size()) | boost::adaptors::reversed)
{
  auto&& arg = _funCall.arguments[i];

? Or a very simple helper that returns boost::counting_range(0, _container.size())?

I could look into helpers that give you tuples of index and value that are robust against reversing - or even helpers that let you iterate two containers simultanously, so you could use auto&& [needsLiteralArgument, arg]: someWrapper(*needsLiteralArguments, _funCall.arguments) | boost::adaptors::reversed - that's possible, but it can get quite messy quickly :-).

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you see the link I posted above?

Copy link
Member

Choose a reason for hiding this comment

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

By the way: the solution in the link always creates a copy - a more sophisticated solution would use references for lvalues and move for rvalues (again part of the messy-ness ;-))

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine as is. i - 1 is only used twice and the for line is very clear

Copy link
Member

Choose a reason for hiding this comment

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

Maybe change i to something else though? i is also used below, might look confusing visually

Copy link
Contributor

Choose a reason for hiding this comment

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

We have tons of other places in the code where an enumerate or "iterate two lists at the same time" would be very handy.

{
Expression const& arg = _funCall.arguments[i - 1];

argTypes.emplace_back(expectExpression(arg));

if (needsLiteralArguments)
if (needsLiteralArguments && (*needsLiteralArguments)[i - 1])
{
if (!holds_alternative<Literal>(arg))
typeError(
Expand Down
5 changes: 3 additions & 2 deletions libyul/Dialect.h
View file
Open in desktop
Expand Up @@ -28,6 +28,7 @@

#include <vector>
#include <set>
#include <optional>

namespace solidity::yul
{
Expand All @@ -46,8 +47,8 @@ struct BuiltinFunction
ControlFlowSideEffects controlFlowSideEffects;
/// If true, this is the msize instruction.
bool isMSize = false;
/// If true, can only accept literals as arguments and they cannot be moved to variables.
bool literalArguments = false;
/// If set, same length as the arguments, if true at index i, the i'th argument has to be a literal which means it can't be moved to variables.
std::optional<std::vector<bool>> literalArguments;
};

struct Dialect: boost::noncopyable
Expand Down
21 changes: 13 additions & 8 deletions libyul/backends/evm/EVMDialect.cpp
View file
Open in desktop
Expand Up @@ -55,7 +55,7 @@ pair<YulString, BuiltinFunctionForEVM> createEVMFunction(
f.controlFlowSideEffects.terminates = evmasm::SemanticInformation::terminatesControlFlow(_instruction);
f.controlFlowSideEffects.reverts = evmasm::SemanticInformation::reverts(_instruction);
f.isMSize = _instruction == evmasm::Instruction::MSIZE;
f.literalArguments = false;
f.literalArguments.reset();
f.instruction = _instruction;
f.generateCode = [_instruction](
FunctionCall const&,
Expand All @@ -75,17 +75,22 @@ pair<YulString, BuiltinFunctionForEVM> createFunction(
size_t _params,
size_t _returns,
SideEffects _sideEffects,
bool _literalArguments,
vector<bool> _literalArguments,
std::function<void(FunctionCall const&, AbstractAssembly&, BuiltinContext&, std::function<void()>)> _generateCode
)
{
solAssert(_literalArguments.size() == _params || _literalArguments.empty(), "");

YulString name{std::move(_name)};
BuiltinFunctionForEVM f;
f.name = name;
f.parameters.resize(_params);
f.returns.resize(_returns);
f.sideEffects = std::move(_sideEffects);
f.literalArguments = _literalArguments;
if (!_literalArguments.empty())
f.literalArguments = std::move(_literalArguments);
else
f.literalArguments.reset();
f.isMSize = false;
f.instruction = {};
f.generateCode = std::move(_generateCode);
Expand All @@ -107,7 +112,7 @@ map<YulString, BuiltinFunctionForEVM> createBuiltins(langutil::EVMVersion _evmVe

if (_objectAccess)
{
builtins.emplace(createFunction("datasize", 1, 1, SideEffects{}, true, [](
builtins.emplace(createFunction("datasize", 1, 1, SideEffects{}, {true}, [](
FunctionCall const& _call,
AbstractAssembly& _assembly,
BuiltinContext& _context,
Expand All @@ -128,7 +133,7 @@ map<YulString, BuiltinFunctionForEVM> createBuiltins(langutil::EVMVersion _evmVe
_assembly.appendDataSize(_context.subIDs.at(dataName));
}
}));
builtins.emplace(createFunction("dataoffset", 1, 1, SideEffects{}, true, [](
builtins.emplace(createFunction("dataoffset", 1, 1, SideEffects{}, {true}, [](
FunctionCall const& _call,
AbstractAssembly& _assembly,
BuiltinContext& _context,
Expand All @@ -154,7 +159,7 @@ map<YulString, BuiltinFunctionForEVM> createBuiltins(langutil::EVMVersion _evmVe
3,
0,
SideEffects{false, false, false, false, true},
false,
{},
[](
FunctionCall const&,
AbstractAssembly& _assembly,
Expand Down Expand Up @@ -262,7 +267,7 @@ EVMDialectTyped::EVMDialectTyped(langutil::EVMVersion _evmVersion, bool _objectA
m_functions["popbool"_yulstring] = m_functions["pop"_yulstring];
m_functions["popbool"_yulstring].name = "popbool"_yulstring;
m_functions["popbool"_yulstring].parameters = {"bool"_yulstring};
m_functions.insert(createFunction("bool_to_u256", 1, 1, {}, false, [](
m_functions.insert(createFunction("bool_to_u256", 1, 1, {}, {}, [](
FunctionCall const&,
AbstractAssembly&,
BuiltinContext&,
Expand All @@ -272,7 +277,7 @@ EVMDialectTyped::EVMDialectTyped(langutil::EVMVersion _evmVersion, bool _objectA
}));
m_functions["bool_to_u256"_yulstring].parameters = {"bool"_yulstring};
m_functions["bool_to_u256"_yulstring].returns = {"u256"_yulstring};
m_functions.insert(createFunction("u256_to_bool", 1, 1, {}, false, [](
m_functions.insert(createFunction("u256_to_bool", 1, 1, {}, {}, [](
FunctionCall const&,
AbstractAssembly& _assembly,
BuiltinContext&,
Expand Down
2 changes: 1 addition & 1 deletion libyul/backends/evm/EVMDialect.h
View file
Open in desktop
Expand Up @@ -45,7 +45,7 @@ struct BuiltinContext
std::map<YulString, AbstractAssembly::SubID> subIDs;
};

struct BuiltinFunctionForEVM: BuiltinFunction
struct BuiltinFunctionForEVM: public BuiltinFunction
{
std::optional<evmasm::Instruction> instruction;
/// Function to generate code for the given function call and append it to the abstract
Expand Down
10 changes: 7 additions & 3 deletions libyul/backends/wasm/WasmCodeTransform.cpp
View file
Open in desktop
Expand Up @@ -136,11 +136,15 @@ wasm::Expression WasmCodeTransform::operator()(FunctionCall const& _call)
}
typeConversionNeeded = true;
}
else if (builtin->literalArguments)
else if (builtin->literalArguments && contains(builtin->literalArguments.value(), true))
{
vector<wasm::Expression> literals;
for (auto const& arg: _call.arguments)
literals.emplace_back(wasm::StringLiteral{std::get<Literal>(arg).value.str()});
for (size_t i = 0; i < _call.arguments.size(); i++)
if (builtin->literalArguments.value()[i])
literals.emplace_back(wasm::StringLiteral{std::get<Literal>(_call.arguments[i]).value.str()});
else
literals.emplace_back(visitReturnByValue(_call.arguments[i]));

return wasm::BuiltinCall{_call.functionName.name.str(), std::move(literals)};
}
else
Expand Down
13 changes: 8 additions & 5 deletions libyul/backends/wasm/WasmDialect.cpp
View file
Open in desktop
Expand Up @@ -102,8 +102,8 @@ WasmDialect::WasmDialect()
m_functions["unreachable"_yulstring].controlFlowSideEffects.terminates = true;
m_functions["unreachable"_yulstring].controlFlowSideEffects.reverts = true;

addFunction("datasize", {i64}, {i64}, true, true);
addFunction("dataoffset", {i64}, {i64}, true, true);
addFunction("datasize", {i64}, {i64}, true, {true});
addFunction("dataoffset", {i64}, {i64}, true, {true});

addEthereumExternals();
}
Expand Down Expand Up @@ -204,7 +204,7 @@ void WasmDialect::addEthereumExternals()
f.controlFlowSideEffects = ext.controlFlowSideEffects;
f.isMSize = false;
f.sideEffects.invalidatesStorage = (ext.name == "storageStore");
f.literalArguments = false;
f.literalArguments.reset();
}
}

Expand All @@ -213,7 +213,7 @@ void WasmDialect::addFunction(
vector<YulString> _params,
vector<YulString> _returns,
bool _movable,
bool _literalArguments
std::vector<bool> _literalArguments
)
{
YulString name{move(_name)};
Expand All @@ -224,5 +224,8 @@ void WasmDialect::addFunction(
f.returns = std::move(_returns);
f.sideEffects = _movable ? SideEffects{} : SideEffects::worst();
f.isMSize = false;
f.literalArguments = _literalArguments;
if (!_literalArguments.empty())
f.literalArguments = std::move(_literalArguments);
else
f.literalArguments.reset();
}
2 changes: 1 addition & 1 deletion libyul/backends/wasm/WasmDialect.h
View file
Open in desktop
Expand Up @@ -61,7 +61,7 @@ struct WasmDialect: public Dialect
std::vector<YulString> _params,
std::vector<YulString> _returns,
bool _movable = true,
bool _literalArguments = false
std::vector<bool> _literalArguments = std::vector<bool>{}
);

std::map<YulString, BuiltinFunction> m_functions;
Expand Down
36 changes: 17 additions & 19 deletions libyul/backends/wasm/WordSizeTransform.cpp
View file
Open in desktop
Expand Up @@ -41,15 +41,24 @@ void WordSizeTransform::operator()(FunctionDefinition& _fd)

void WordSizeTransform::operator()(FunctionCall& _fc)
{
vector<bool> const* literalArguments = nullptr;

if (BuiltinFunction const* fun = m_inputDialect.builtin(_fc.functionName.name))
if (fun->literalArguments)
literalArguments = &fun->literalArguments.value();

vector<Expression> newArgs;

for (size_t i = 0; i < _fc.arguments.size(); i++)
if (!literalArguments || !(*literalArguments)[i])
newArgs += expandValueToVector(_fc.arguments[i]);
else
{
for (Expression& arg: _fc.arguments)
get<Literal>(arg).type = m_targetDialect.defaultType;
return;
get<Literal>(_fc.arguments[i]).type = m_targetDialect.defaultType;
newArgs.emplace_back(std::move(_fc.arguments[i]));
}

rewriteFunctionCallArguments(_fc.arguments);
_fc.arguments = std::move(newArgs);
}

void WordSizeTransform::operator()(If& _if)
Expand Down Expand Up @@ -97,9 +106,9 @@ void WordSizeTransform::operator()(Block& _block)

// Special handling for datasize and dataoffset - they will only need one variable.
if (BuiltinFunction const* f = m_inputDialect.builtin(std::get<FunctionCall>(*varDecl.value).functionName.name))
if (f->literalArguments)
if (f->name == "datasize"_yulstring || f->name == "dataoffset"_yulstring)
Marenz marked this conversation as resolved.
Show resolved Hide resolved
{
yulAssert(f->name == "datasize"_yulstring || f->name == "dataoffset"_yulstring, "");
yulAssert(f->literalArguments && f->literalArguments.value()[0], "");
yulAssert(varDecl.variables.size() == 1, "");
auto newLhs = generateU64IdentifierNames(varDecl.variables[0].name);
vector<Statement> ret;
Expand Down Expand Up @@ -158,9 +167,9 @@ void WordSizeTransform::operator()(Block& _block)

// Special handling for datasize and dataoffset - they will only need one variable.
if (BuiltinFunction const* f = m_inputDialect.builtin(std::get<FunctionCall>(*assignment.value).functionName.name))
if (f->literalArguments)
if (f->name == "datasize"_yulstring || f->name == "dataoffset"_yulstring)
{
yulAssert(f->name == "datasize"_yulstring || f->name == "dataoffset"_yulstring, "");
yulAssert(f->literalArguments && f->literalArguments.value()[0], "");
yulAssert(assignment.variableNames.size() == 1, "");
auto newLhs = generateU64IdentifierNames(assignment.variableNames[0].name);
vector<Statement> ret;
Expand Down Expand Up @@ -268,17 +277,6 @@ void WordSizeTransform::rewriteIdentifierList(vector<Identifier>& _ids)
);
}

void WordSizeTransform::rewriteFunctionCallArguments(vector<Expression>& _args)
{
iterateReplacing(
_args,
[&](Expression& _e) -> std::optional<vector<Expression>>
{
return expandValueToVector(_e);
}
);
}

vector<Statement> WordSizeTransform::handleSwitchInternal(
langutil::SourceLocation const& _location,
vector<YulString> const& _splitExpressions,
Expand Down
1 change: 0 additions & 1 deletion libyul/backends/wasm/WordSizeTransform.h
View file
Open in desktop
Expand Up @@ -83,7 +83,6 @@ class WordSizeTransform: public ASTModifier

void rewriteVarDeclList(std::vector<TypedName>&);
void rewriteIdentifierList(std::vector<Identifier>&);
void rewriteFunctionCallArguments(std::vector<Expression>&);

std::vector<Statement> handleSwitch(Switch& _switch);
std::vector<Statement> handleSwitchInternal(
Expand Down
15 changes: 12 additions & 3 deletions libyul/optimiser/CommonSubexpressionEliminator.cpp
View file
Open in desktop
Expand Up @@ -58,12 +58,21 @@ void CommonSubexpressionEliminator::visit(Expression& _e)
// If this is a function call to a function that requires literal arguments,
// do not try to simplify there.
if (holds_alternative<FunctionCall>(_e))
if (BuiltinFunction const* builtin = m_dialect.builtin(std::get<FunctionCall>(_e).functionName.name))
if (builtin->literalArguments)
{
FunctionCall& funCall = std::get<FunctionCall>(_e);

if (BuiltinFunction const* builtin = m_dialect.builtin(funCall.functionName.name))
{
for (size_t i = funCall.arguments.size(); i > 0; i--)
// We should not modify function arguments that have to be literals
// Note that replacing the function call entirely is fine,
// if the function call is movable.
descend = false;
if (!builtin->literalArguments || !builtin->literalArguments.value()[i - 1])
visit(funCall.arguments[i - 1]);

descend = false;
}
}

// We visit the inner expression first to first simplify inner expressions,
// which hopefully allows more matches.
Expand Down
10 changes: 6 additions & 4 deletions libyul/optimiser/ExpressionSplitter.cpp
View file
Open in desktop
Expand Up @@ -47,13 +47,15 @@ void ExpressionSplitter::run(OptimiserStepContext& _context, Block& _ast)

void ExpressionSplitter::operator()(FunctionCall& _funCall)
{
vector<bool> const* literalArgs = nullptr;

if (BuiltinFunction const* builtin = m_dialect.builtin(_funCall.functionName.name))
if (builtin->literalArguments)
// We cannot outline function arguments that have to be literals
return;
literalArgs = &builtin->literalArguments.value();

for (auto& arg: _funCall.arguments | boost::adaptors::reversed)
outlineExpression(arg);
for (size_t i = _funCall.arguments.size(); i > 0; i--)
if (!literalArgs || !(*literalArgs)[i - 1])
outlineExpression(_funCall.arguments[i - 1]);
}

void ExpressionSplitter::operator()(If& _if)
Expand Down
2 changes: 1 addition & 1 deletion test/libyul/Parser.cpp
View file
Open in desktop
Expand Up @@ -539,7 +539,7 @@ BOOST_AUTO_TEST_CASE(builtins_analysis)
{
return _name == "builtin"_yulstring ? &f : nullptr;
}
BuiltinFunction f{"builtin"_yulstring, vector<Type>(2), vector<Type>(3), {}, {}};
BuiltinFunction f{"builtin"_yulstring, vector<Type>(2), vector<Type>(3), {}, {}, false, {}};
};

SimpleDialect dialect;
Expand Down