-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
[test] Add support to run yul optimizer assembly test. #15009
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this is pretty much an exact copy of YulOptimizerTest.h
at this point, so I assume it's unfinished. What is actually the idea behind this test? Just displaying the assembly next to the optimized Yul or something more?
|
||
// Re-parse new code for compilability | ||
// TODO: support for wordSizeTransform which needs different input and output dialects | ||
if (m_optimizerStep != "wordSizeTransform" && !std::get<0>(parse(_stream, _linePrefix, _formatted, printed))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wordSizeTransform
seems to be some Ewasm left-over. We have no such step now.
I think we can safely remove this from YulOptimizerTest
and a few other places that still reference it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was marked as resolved, but this code is still here.
e953eca
to
917cec1
Compare
8a4d556
to
de39abd
Compare
de39abd
to
9357780
Compare
Just to answer this: the target here is to test the transport of the debugging context through any individual optimizer step and then to assembly (which is from where we'll output them in the end) In any case, doesn't hurt to have, and not too much of a headache to just merge. |
|
||
// Re-parse new code for compilability | ||
// TODO: support for wordSizeTransform which needs different input and output dialects | ||
if (m_optimizerStep != "wordSizeTransform" && !std::get<0>(parse(_stream, _linePrefix, _formatted, printed))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was marked as resolved, but this code is still here.
auto dialectName = m_reader.stringSetting("dialect", "evm"); | ||
m_dialect = &dialect(dialectName, solidity::test::CommonOptions::get().evmVersion()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Ewasm gone, adding an option for dialect selection in a newly written test case seems pointless to me. Technically we still support the evmTyped
and yul
dialects, but have you even checked if they work here? That would be a waste of time anyway and IMO would be much better to just remove this option and assume the evm
dialect.
You could then also get rid of dialect checks in the rest of the test case.
EVMObjectCompiler::compile( | ||
*m_object, | ||
adapter, | ||
EVMDialect::strictAssemblyForEVMObjects(EVMVersion{}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why aren't we respecting the dialect and EVM version selected by the user here even though we do in the other places in this test case?
Same question for analyzeStrictAssertCorrect()
above.
Also, it would be a good idea to have something EVM version-specific in the sample test case so that we can see if this works correctly.
{ | ||
let _1 := mload(0) | ||
let f_a := mload(1) | ||
let f_r | ||
{ | ||
f_a := mload(f_a) | ||
f_r := add(f_a, calldatasize()) | ||
} | ||
let z := mload(2) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if the new test is meant for inspecting the debug data, then it would be useful to actually have some debug annotations in the sample test to see if it's even preserved in the output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now it is only meant to print the assembly in this PR. The printing of the debug data is so far done in #14969 - but I guess some changes are still needed there.
false, | ||
std::nullopt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
false, | |
std::nullopt | |
false, // _optimize | |
std::nullopt // _eofVersion |
I think it this should be using the value supplied to --eof-version
(i.e. solidity::test::CommonOptions::get().eofVersion()
). Which will still be nullopt
, by default, but I see no reason to ignore this option if supplied.
As for optimization, I guess we don't want because that's only the legacy optimizer? If so, at least a comment saying that would be nice. Otherwise these choices seem arbitrary.
std::ostringstream output; | ||
output << assembly; | ||
m_obtainedResult += "\nAssembly:\n" + output.str(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern is so common that we already have a helper for it in CommonIO.h
:
std::ostringstream output; | |
output << assembly; | |
m_obtainedResult += "\nAssembly:\n" + output.str(); | |
m_obtainedResult += "\nAssembly:\n" + toString(assembly); |
|
||
TestCase::TestResult YulOptimizerAssemblyTest::run(std::ostream& _stream, std::string const& _linePrefix, bool const _formatted) | ||
{ | ||
std::tie(m_object, m_analysisInfo) = parse(_stream, _linePrefix, _formatted, m_source); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_object
and m_analysisInfo
are never used outside of this function. What's the point of even having them as class members rather than locals?
std::pair<std::shared_ptr<Object>, std::shared_ptr<AsmAnalysisInfo>> YulOptimizerAssemblyTest::parse( | ||
std::ostream& _stream, | ||
std::string const& _linePrefix, | ||
bool const _formatted, | ||
std::string const& _source | ||
) | ||
{ | ||
ErrorList errors; | ||
soltestAssert(m_dialect, ""); | ||
std::shared_ptr<Object> object; | ||
std::shared_ptr<AsmAnalysisInfo> analysisInfo; | ||
std::tie(object, analysisInfo) = yul::test::parse(_source, *m_dialect, errors); | ||
if (!object || !analysisInfo || Error::containsErrors(errors)) | ||
{ | ||
AnsiColorized(_stream, _formatted, {formatting::BOLD, formatting::RED}) << _linePrefix << "Error parsing source." << std::endl; | ||
CharStream charStream(_source, ""); | ||
SourceReferenceFormatter{_stream, SingletonCharStreamProvider(charStream), true, false} | ||
.printErrorInformation(errors); | ||
return {}; | ||
} | ||
return {std::move(object), std::move(analysisInfo)}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it was copied verbatim from YulOptimizerTest
. Why not just reuse it? Other than m_dialect
(which could be parameter), this could easily be converted into a static function or a standalone helper and just called by this class.
m_object->analysisInfo = std::make_shared<AsmAnalysisInfo>( | ||
AsmAnalyzer::analyzeStrictAssertCorrect(EVMDialect::strictAssemblyForEVMObjects(solidity::test::CommonOptions::get().evmVersion()), *m_object) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really correct to simply overwrite the previous analysis info? Do we do that in real compilation too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basically this is needed, because after the optimisation step the code might have slightly changed, so we need to update the analysis info. if this is not done, the evm code transform will fail.
7f1e1f4
to
8740a1b
Compare
8740a1b
to
b95b101
Compare
This pull request is stale because it has been open for 14 days with no activity. |
This is needed in the context of
ethdebug
. I will need to write some tests on how optimiser steps are dealing with the debug attributes defined by #14857. The ideas is to implement different PRs per optimiser step on top of #14969, where this PR will enable the visualisation how these debug attributes got potentially merged - depending on the optimiser step.For context, see e.g. #15087.