Skip to content

Introduce JumpdestRemover optimisation step#2657

Merged
axic merged 3 commits intodevelopfrom
jumpdest-remover
Aug 25, 2017
Merged

Introduce JumpdestRemover optimisation step#2657
axic merged 3 commits intodevelopfrom
jumpdest-remover

Conversation

@axic
Copy link
Contributor

@axic axic commented Jul 27, 2017

No description provided.

@axic
Copy link
Contributor Author

axic commented Jul 27, 2017

Some of the bigger assemblies in the Parity multisig (assembly items: before, after):

5474 -> 5430
3857 -> 3802
3638 -> 3635
3616 -> 3616

The function dispatcher leaves a lot of stray jumpdests, we could fix that or could keep a simple remover such as this.

@axic axic force-pushed the jumpdest-remover branch from e2d8129 to 6352e25 Compare July 27, 2017 22:39
@axic
Copy link
Contributor Author

axic commented Jul 27, 2017

Haven't bothered updating the tests which check the exact bytecode before the code is accepted.

@axic
Copy link
Contributor Author

axic commented Jul 28, 2017 •
edited
Loading

Probably this should be run again after dedup/CSE.

@chriseth
Copy link
Contributor

chriseth commented Jul 28, 2017

This step was previously done by the control flow graph optimizer, but it was removed because we currently cannot assume that labels that are never pushed can be removed, because they can be stored in storage. If we do a full analysis also passing in referenced labels "from above", it might work. In any case, there should be a test that fails if you do not consider this.

@axic
Copy link
Contributor Author

axic commented Jul 28, 2017

we currently cannot assume that labels that are never pushed can be removed, because they can be stored in storage

Hm, yes that's true, this change would break that.

Thinking a bit more about that, is that valid usage though? If it is at any point of time pushed to storage in the code/constructor, it is already safe.

In an example like { 4 calldataload jump } , where the tag is received from the calldata it wouldn't work. The question is: is that a valid use case we want to support? I don't think so.

@axic axic force-pushed the jumpdest-remover branch from 6352e25 to afebbd8 Compare July 28, 2017 15:49
@chriseth
Copy link
Contributor

chriseth commented Jul 28, 2017

When the jumpdest remover as currently implemented analyses the runtime assembly, it does not consider the jumpdests from the runtime assembly that were accessed in the creation time assembly.

@axic
Copy link
Contributor Author

axic commented Jul 28, 2017

that were accessed in the creation time assembly.

True. Is that an actual case we have? I guess if helpers are used in a constructor which are also used in runtime functions. Strange no tests caught this, should add a test for it.

@chriseth
Copy link
Contributor

chriseth commented Jul 28, 2017

The test store_internal_unused_function_in_constructor should fail.

@axic
Copy link
Contributor Author

axic commented Jul 28, 2017

Doesn't seem to fail on that one.

@axic axic changed the title Introduce JumpdestRemover optimisation step [WIP] Introduce JumpdestRemover optimisation step Aug 10, 2017
@chriseth
Copy link
Contributor

chriseth commented Aug 15, 2017

As can be seen here - https://medium.com/@hayeah/diving-into-the-ethereum-vm-part-2-storage-layout-bc5349cb11b7 - it seems we have a lot of jumpdests that hinder peephole or CSE optimization, so increasing the priority of this.

chriseth
chriseth reviewed Aug 15, 2017
View reviewed changes
libevmasm/Assembly.cpp Outdated
}
}

/// Run again after Deduplicate or CSE
Copy link
Contributor

@chriseth chriseth Aug 15, 2017

Choose a reason for hiding this comment

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

why is this needed? We will just have another iteration of the loop, won't we?

Copy link
Contributor Author

@axic axic Aug 15, 2017

Choose a reason for hiding this comment

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

I've seen some cases where dedup/cse left unused tags. Admittedly they could be improved to not do that.

@chriseth
Copy link
Contributor

chriseth commented Aug 15, 2017

Ok, now I'm relieved: The test does actually fail.

@chriseth chriseth force-pushed the jumpdest-remover branch 2 times, most recently from f5a5345 to 3b9b103 Compare August 15, 2017 13:11
@axic
Copy link
Contributor Author

axic commented Aug 15, 2017

Can you add some specific tests (including subassemblies) to test/libevmasm/Optimiser?

@chriseth
Copy link
Contributor

chriseth commented Aug 15, 2017

Added test.

@chriseth chriseth changed the title [WIP] Introduce JumpdestRemover optimisation step Introduce JumpdestRemover optimisation step Aug 15, 2017
axic
axic commented Aug 15, 2017
View reviewed changes
libevmasm/JumpdestRemover.h Outdated

AssemblyItems& m_items;
bool mutable m_initialized = false;
std::set<std::pair<size_t, size_t>> mutable m_referencedTags;
Copy link
Contributor Author

@axic axic Aug 15, 2017

Choose a reason for hiding this comment

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

These two are unused.

axic
axic commented Aug 15, 2017
View reviewed changes
libevmasm/JumpdestRemover.h Outdated
static std::set<size_t> referencedTags(AssemblyItems const& _items, size_t _subId);

private:
void analyzeIfNecessary() const;
Copy link
Contributor Author

@axic axic Aug 15, 2017

Choose a reason for hiding this comment

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

Not used.

@chriseth
Copy link
Contributor

chriseth commented Aug 15, 2017

Weird that the compiler does not warn about that...

@chriseth
Copy link
Contributor

chriseth commented Aug 16, 2017

This is ready to be merged.

@axic
Copy link
Contributor Author

axic commented Aug 21, 2017

@chriseth as discussed last meeting we wanted to run the truffle tests on openzeppelin with this change.

@chriseth
Copy link
Contributor

chriseth commented Aug 21, 2017

What about merging and then testing the nightly?

@axic
Copy link
Contributor Author

axic commented Aug 23, 2017

Depends on #2782.

@axic axic force-pushed the jumpdest-remover branch from 80a865b to a688b9f Compare August 23, 2017 22:57
@axic axic force-pushed the jumpdest-remover branch from a6a4dee to 223235c Compare August 25, 2017 09:43
@axic
Copy link
Contributor Author

axic commented Aug 25, 2017

Rebased, merging this next.

@axic axic merged commit 38035f8 into develop Aug 25, 2017
@axic axic deleted the jumpdest-remover branch August 25, 2017 10:20
@axic axic removed the nextrelease label Aug 25, 2017
@axic axic mentioned this pull request Oct 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@chriseth chriseth chriseth approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@axic @chriseth