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

[WIP] Struct encoder #2433

Closed
wants to merge 13 commits into from
Closed

[WIP] Struct encoder #2433

wants to merge 13 commits into from

Conversation

chriseth
Copy link
Contributor

@chriseth chriseth commented Jun 21, 2017

  • reduce duplication of generated code
  • simple JULIA optimizer stage
  • check where ABI encoders from storage are used, verify code coverage and add tests if needed

Currently, we can only reduce duplication if all encoders are handled in a single pass of the JULIA parser, because only then we can have different JULIA functions referencing themselves. This is only possible if the ABI encoder has a label we can jump to. This requires the following refactorings:

Each call to encodeToMemory has to be changed to an actual function call and for that, the return label has to be put onto the stack before the values to encode are put onto the stack.

Furthermore, we have to have a way to reference a label in a piece of JULIA code from outside of that JULIA code and this also has to be possible before that code is even parsed.

I think the best solution to this problem is to provide a string as a hint to AbstractAssembly::newLabelID so that the EVM assembly can specially prepare some label ids to be linked properly.

Depends on #1673 and #2704.

@chriseth chriseth changed the title Struct encoder [WIP] Struct encoder Jun 21, 2017
values[i]["i"] = to_string(i);
values[i]["headPos"] = to_string(headPos);
headPos += _targetTypes[i]->calldataEncodedSize();
string encodingFuctionName = _givenTypes[i]->identifier() + "_TO_" + _targetTypes[i]->identifier();
Copy link
Member

@axic axic Jun 22, 2017

Choose a reason for hiding this comment

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

The function uses lowercase _to_. Actually this isn't even used, wouldn't a list suffice?

@axic
Copy link
Member

axic commented Jun 26, 2017

Please rebase to use Whiskers 😉

@chriseth chriseth force-pushed the structEncoder branch 2 times, most recently from cf1a667 to 705053a Compare June 29, 2017 15:52
@chriseth
Copy link
Contributor Author

Only fails 105 out of 422 end to end test! :-)

@chriseth
Copy link
Contributor Author

Down to 69

@chriseth chriseth mentioned this pull request Jun 29, 2017
2 tasks
@@ -180,6 +181,14 @@ void CompilerUtils::encodeToMemory(
t = t->mobileType()->interfaceType(_encodeAsLibraryTypes)->encodingType();
}

if (_givenTypes.empty())
Copy link
Member

Choose a reason for hiding this comment

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

Can we merge this optimisation right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The optimizer (if not the peephole optimizer) will fix that, but yes, I can try to extract it.

@@ -311,7 +311,7 @@ void ContractCompiler::appendCalldataUnpacker(TypePointers const& _typeParameter
{
// stack: v1 v2 ... v(k-1) base_offset current_offset
TypePointer type = parameterType->decodingType();
solAssert(type, "No decoding type found.");
solUnimplementedAssert(type, "No decoding type found.");
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps take this with the other change too (in general I like those specific solUnimplementedAsserts better from a user's perspective).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this has to be an assert in current develop, I changed it to an unimplemented because of the new struct types.

@axic
Copy link
Member

axic commented Jun 30, 2017

Please rebase.

@chriseth chriseth force-pushed the structEncoder branch 2 times, most recently from a7e0193 to dfd5aba Compare July 1, 2017 15:59
@chriseth
Copy link
Contributor Author

chriseth commented Jul 1, 2017

Down to 61 errors in SolidityEndToEndTests.

@chriseth
Copy link
Contributor Author

chriseth commented Jul 1, 2017

By the way, an example encoder now looks as follows:

{
        let dynFree := add($headStart, 64)
        dynFree := abi_encode_t_bytes2_to_t_bytes3($value0, $headStart, add($headStart, 0), dynFree)
        dynFree := abi_encode_t_bool_to_t_bool($value1, $headStart, add($headStart, 32), dynFree)
        $value0 := dynFree
        function abi_encode_t_bool_to_t_bool(value, headStart, headPos, dyn) -> newDyn
        {
            newDyn := dyn
            mstore(headPos, cleanup_assert_t_bool(value))
        }
        function abi_encode_t_bytes2_to_t_bytes3(value, headStart, headPos, dyn) -> newDyn
        {
            newDyn := dyn
            mstore(headPos, convert_t_bytes2_to_t_bytes3(value))
        }
        function cleanup_assert_t_bool(value) -> cleaned
        {
            cleaned := iszero(iszero(value))
        }
        function cleanup_assert_t_bytes2(value) -> cleaned
        {
            cleaned := and(value, 0xFFFF000000000000000000000000000000000000000000000000000000000000)
        }
        function convert_t_bytes2_to_t_bytes3(value) -> converted
        {
            converted := cleanup_assert_t_bytes2(value)
        }
    }

I.e. it has very detailed "convert" and "cleanup" calls which makes it very visible what happens and those can also probably be easily inlined.

@chriseth
Copy link
Contributor Author

chriseth commented Jul 3, 2017

59!

@chriseth
Copy link
Contributor Author

chriseth commented Aug 2, 2017

24!

@chriseth
Copy link
Contributor Author

chriseth commented Aug 2, 2017

14!

@chriseth
Copy link
Contributor Author

chriseth commented Aug 3, 2017

Only one failing test left, but this is due to the fact that we do not use codecopy for constants, but I think this will also turn green once we have deduplication.

Still, more test coverage is needed.

@chriseth
Copy link
Contributor Author

chriseth commented Aug 3, 2017

Oh and it turns out, the storage-to-ABI-encoder is actually used, but it seems there are only few tests for it.

@@ -287,7 +287,7 @@ class ExecutionFramework
Address m_contractAddress;
u256 m_blockNumber;
u256 const m_gasPrice = 100 * szabo;
u256 const m_gas = 100000000;
u256 m_gas = 100000000;
Copy link
Member

Choose a reason for hiding this comment

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

Where is this modified?

@axic
Copy link
Member

axic commented Aug 3, 2017

Can you pull the tests which should succeed on the current encoder into a separate PR and merge them first?

@chriseth chriseth mentioned this pull request Aug 7, 2017
@axic axic mentioned this pull request Aug 25, 2017
2 tasks
@chriseth chriseth closed this Sep 14, 2017
@axic axic deleted the structEncoder branch September 14, 2017 16:50
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.

None yet

2 participants