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

Rematerialisation step. #3332

Merged
merged 10 commits into from Feb 6, 2018

Conversation

Projects
None yet
3 participants
@chriseth
Contributor

chriseth commented Dec 14, 2017

Depends on #3317 and #3352.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Dec 14, 2017

Contributor

I think I will also add the implementation to this PR, perhaps refining and changing the description in the process.

Contributor

chriseth commented Dec 14, 2017

I think I will also add the implementation to this PR, perhaps refining and changing the description in the process.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Dec 15, 2017

Contributor

This should work, but I would feel much more comfortable if we required a conversion to SSA first.

The main downside of SSA is that it requires either a re-unification of variable names or much more logic in the assembly stage (which I would rather avoid). Also it might destroy debug information about the stack location of local variables.

Perhaps we could try to keep some relation between the different variants of the same variable (i.e. by suffixing every variable by _i) and then write the reunification in a way that re-using variable names that have the same prefix is preferred.

Contributor

chriseth commented Dec 15, 2017

This should work, but I would feel much more comfortable if we required a conversion to SSA first.

The main downside of SSA is that it requires either a re-unification of variable names or much more logic in the assembly stage (which I would rather avoid). Also it might destroy debug information about the stack location of local variables.

Perhaps we could try to keep some relation between the different variants of the same variable (i.e. by suffixing every variable by _i) and then write the reunification in a way that re-using variable names that have the same prefix is preferred.

@chriseth chriseth changed the title from Description of rematerialisation and elimination step. to Rematerialisation step. Dec 18, 2017

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Jan 6, 2018

Member

I think this PR doesn't even need the last two commits ("References counter." and
"Use external variable reference counter component.") because those are part of the #3351?

Member

axic commented Jan 6, 2018

I think this PR doesn't even need the last two commits ("References counter." and
"Use external variable reference counter component.") because those are part of the #3351?

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Feb 6, 2018

Member

Needs to be rebased.

Member

axic commented Feb 6, 2018

Needs to be rebased.

bool inScope(std::string const& _variableName) const;
/// Current values of variables, always movable.
std::map<std::string, Expression const*> m_value;

This comment has been minimized.

@axic

axic Feb 6, 2018

Member

m_values?

@axic

axic Feb 6, 2018

Member

m_values?

}
solAssert(m_value.at(name), "");
auto const& value = *m_value.at(name);
if (expressionValid && CodeSize::codeSize(value) <= 7)

This comment has been minimized.

@axic

axic Feb 6, 2018

Member

Add a FIXME here to adjust this once the CodeSize metric has changed.

@axic

axic Feb 6, 2018

Member

Add a FIXME here to adjust this once the CodeSize metric has changed.

This comment has been minimized.

@axic

axic Feb 6, 2018

Member

Not needed since we have test cases for both below and above the metric.

@axic

axic Feb 6, 2018

Member

Not needed since we have test cases for both below and above the metric.

"add(mul(calldatasize(), 2), number())"
") }"
);
}

This comment has been minimized.

@axic

axic Feb 6, 2018

Member

Need to add another test which has a codesize value of > 7 so it is not replaced.

@axic

axic Feb 6, 2018

Member

Need to add another test which has a codesize value of > 7 so it is not replaced.

This comment has been minimized.

@chriseth

chriseth Feb 6, 2018

Contributor

added

@chriseth

chriseth Feb 6, 2018

Contributor

added

@axic

axic approved these changes Feb 6, 2018

@axic axic merged commit d786d65 into develop Feb 6, 2018

1 of 3 checks passed

ci/circleci Your tests failed on CircleCI
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@axic axic deleted the elimination_descirption branch Feb 6, 2018

@axic axic removed the in progress label Feb 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment