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

Integrate opcode semantics summaries #2728

Merged
merged 6 commits into from
Apr 2, 2025
Merged

Integrate opcode semantics summaries #2728

merged 6 commits into from
Apr 2, 2025

Conversation

Stevengre
Copy link
Contributor

  • Refactored the summarize function to print correct and compilable k definitions for summaries.
  • Added new summary targets for LLVM and Haskell backend in the KEVM plugin.
  • Updated the Makefile to include a new test target for test-prove-summaries.
  • Refactored the test suite to validate opcode summaries.
  • Adjusted imports in the driver and EDSL modules to incorporate the new summaries.
  • Cleaned up the .gitignore to ensure proper tracking of relevant files.

@Stevengre Stevengre self-assigned this Mar 24, 2025
@Stevengre Stevengre marked this pull request as ready for review March 24, 2025 14:34
@Stevengre
Copy link
Contributor Author

Time cost with optimizations

  • make test-conformance
real    3m41.335s
user    11m11.378s
sys     1m37.351s
  • make test-prove-rules PYTEST_PARALLEL=4
real    45m52.965s
user    218m58.204s
sys     11m16.987s
  • make test-prove-functional PYTEST_PARALLEL=4
real    19m10.969s
user    139m9.541s
sys     7m48.138s
  • make test-prove-dss PYTEST_PARALLEL=4
real    20m11.365s
user    50m58.724s
sys     3m30.814s

@nishantjr
Copy link
Contributor

nishantjr commented Mar 25, 2025

@Stevengre Would it be possible to provide a kevm-pyk run --target for haskell-summary and llvm-summary in kevm-pyk/src/kevm_pyk/cli.py?

@Stevengre
Copy link
Contributor Author

Stevengre commented Mar 26, 2025

Time cost with summaries

  • make test-conformance
real    3m36.941s
user    11m6.918s
sys     1m38.048s
  • make test-prove-functional PYTEST_PARALLEL=4
real    34m26.980s
user    166m23.026s
sys     9m36.769s
  • make test-prove-dss PYTEST_PARALLEL=4
real    23m18.205s
user    60m15.341s
sys     4m19.377s

@Stevengre Stevengre force-pushed the integrate-summaries branch from 81ee2cd to cd037a8 Compare March 26, 2025 10:30
@Stevengre
Copy link
Contributor Author

@nishantjr Sure! The new commit provides these new targets! Hope this change is what you want!

@nishantjr
Copy link
Contributor

nishantjr commented Mar 26, 2025

Thank you! It is!

- Refactored the summarize function to print correct and compilable k definitions for summaries.
- Added new summary targets for  LLVM and Haskell backend in the KEVM plugin.
- Updated the Makefile to include a new test target for `test-prove-summaries`.
- Refactored the test suite to validate opcode summaries.
- Adjusted imports in the driver and EDSL modules to incorporate the new summaries.
- Cleaned up the `.gitignore` to ensure proper tracking of relevant files.
…and Haskell. This update expands the choices available for the `--target` argument to include `llvm-summary` and `haskell-summary`.
@Stevengre Stevengre force-pushed the integrate-summaries branch from cd037a8 to 0bfbf95 Compare March 28, 2025 11:16
@Stevengre Stevengre requested a review from JuanCoRo March 31, 2025 15:09
Comment on lines +80 to +81
test-prove-summaries: poetry
$(MAKE) -C kevm-pyk/ test-integration PYTEST_ARGS+="-k test_prove_summaries"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we enable these tests on CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are already summarization tests on CI? Should I delete the summarization process tests and use this one instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should I delete the summarization process tests and use this one instead?

Are these two targets interchangeable? What's the difference between test-summarize and test-prove-summaries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • test-prove-summaries verifies the generated rules in the summaries folder.
  • test-summarize runs the summarization process and validate the generated kcfg using some assertions.

test-prove-summaries provides better assurance for the generated summaries, test-summarize ensures that the new introduced semantics doesn't harm the generation of the summaries.

I think the second target (target for test-summarize) is not quite useful as test-prove-summaries does. It more like update-expected.

So, we can use test-prove-summaries to replace test-summarize to verify the result of the summarization.


ensure_dir_path(self.save_directory)
with open(self.save_directory / f'{proof.id.lower().replace("_", "-")}-summary.k', 'w') as f:
requires = [KRequire('../evm.md')]
Copy link
Contributor

Choose a reason for hiding this comment

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

Model instantiation can be moved out of the context manager's scope.

kdef = KDefinition(module_name, [module], requires=requires)
f.write(self.kevm.pretty_print(kdef))
with open(self.save_directory / 'summaries.k', 'w') as f:
k_files = sorted([f for f in self.save_directory.glob('*.k') if f.name != 'summaries.k'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

- Changed the test suite for the 'Summarization' job from 'test-summarize' to 'test-prove-summaries'.
- Adjusted timeout and parallel execution settings for the 'Summarization' job to improve performance.
- Updated the build command for the distribution to include 'evm-semantics.haskell-summary' instead of 'evm-semantics.summary'.
…instantiation out of the context manager's scope.
@@ -607,6 +607,110 @@ def create_kcfg_explore() -> KCFGExplore:
f.write('\n')
return passed

def _transform_underscores(self, inner: KInner) -> KInner:
Copy link
Contributor

Choose a reason for hiding this comment

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

None of these methods refer to self, so they could be @staticmethod-s, or preferably, even module-level functions.

Comment on lines 725 to 729
body = self._transform_underscores(body)
requires, ensures = self._transform_underscores(requires), self._transform_underscores(ensures)
body, requires = self._transform_inf_gas(rule_id, body, requires)
body, requires = self._transform_dot_account_var(body, requires)
body, requires = self._transform_lhs_functions(rule_id, body, requires)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks pretty neat. ✨

Comment on lines 738 to 746
module_name = f'{proof.id.upper().replace("_", "-")}-SUMMARY'
requires = [KRequire('../evm.md')]
imports = [KImport('EVM')]
if proof.id == 'MSTORE8':
requires.append(KRequire('../buf.md'))
imports.append(KImport('BUF'))
sentences = self._to_rules(proof)
module = KFlatModule(module_name, sentences=sentences, imports=imports)
kdef = KDefinition(module_name, [module], requires=requires)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider extracting a helper.

Comment on lines 750 to 755
k_files = sorted([f for f in self.save_directory.glob('*.k') if f.name != 'summaries.k'])
module_names = [f.stem.upper() for f in k_files]
requires = [KRequire(k_file.name) for k_file in k_files]
imports = [KImport(module_name) for module_name in module_names]
module = KFlatModule('KEVM-SUMMARIES', imports=imports)
kdef = KDefinition('KEVM-SUMMARIES', [module], requires=requires)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here as well.

…functions to get the summary kdefinition for printing.
@Stevengre Stevengre merged commit bf5a247 into master Apr 2, 2025
12 checks passed
@Stevengre Stevengre deleted the integrate-summaries branch April 2, 2025 12:42
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.

3 participants