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

Introducing Ordinal KORE Attribute #1053

Merged
merged 21 commits into from
May 17, 2024
Merged

Introducing Ordinal KORE Attribute #1053

merged 21 commits into from
May 17, 2024

Conversation

Robertorosmaninho
Copy link
Collaborator

@Robertorosmaninho Robertorosmaninho commented May 12, 2024

Closes https://github.com/Pi-Squared-Inc/pi2/issues/1432
This PR introduces a new attribute specific to the LLVM Backend. It includes the ordinal attribute to the KORE definition when preprocess is called after parsing a definition. The content of the attribute is a string_sort with the value of the computed axiom_ordinal.

A new C++ tool is planned to consume this feature instead of reading a definition with a bash script.
The main intention of this feature is to expose this information to the Python bindings to be used in Proof Hints integration tests.

@Robertorosmaninho Robertorosmaninho self-assigned this May 12, 2024
@rv-jenkins rv-jenkins changed the base branch from master to develop May 12, 2024 23:38
Copy link
Collaborator

@dwightguth dwightguth left a comment

Choose a reason for hiding this comment

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

this looks okay but I would want to make sure we've tested that the resulting attribute can be successfully grabbed by the python code that is consuming this before we merge this.

@Robertorosmaninho
Copy link
Collaborator Author

Robertorosmaninho commented May 15, 2024

@dwightguth, I added some Python tests that use the new ordinal attribute.

  • Some things to consider about this approach with Ordinal Attributes:

    • A downside of this approach is to perform a string comparison.
    • The upside is that we don't have to modify any K definitions to make that work.
  • The other approach doesn't actually need the ordinal attribute; instead, we would need to create a new map <rule_label, axiom> and a function get_axiom_from_rule_label to avoid a linear search on the axioms every time we want to find a rule.

    • The downsides of this approach are that we must carry a new Map in every kore_definition, modify K definitions, and enforce that all rules we want to test must have a rule label.
    • The upside is that we can compare the ordinals instead of comparing strings.

@Robertorosmaninho Robertorosmaninho marked this pull request as ready for review May 15, 2024 02:40
include/kllvm/codegen/ProofEvent.h Outdated Show resolved Hide resolved
include/kllvm/ast/AST.h Outdated Show resolved Hide resolved
test/python/test_proof_trace_slow.py Outdated Show resolved Hide resolved
@Baltoli
Copy link
Contributor

Baltoli commented May 15, 2024

I think the ordinal attribute approach is fine; this is ultimately testing infrastructure and so the performance of a string comparison isn't critical. Avoiding modifications of the K source code is nice.

rv-jenkins pushed a commit that referenced this pull request May 16, 2024
…ore definition generation (#1059)

We have seen that the `definition.kore` has been produced in a different
order depending on the OS, which creates a flaky error for some tools
like `llvm-komplie-compute-ordinal` and `llvm-kompile-compute-loc`, an
error on #1053 MacOS tests, and some frontend errors as well.

This PR aims to fix that issue by replacing the data structure used to
store the attributes.
Copy link
Contributor

@Baltoli Baltoli left a comment

Choose a reason for hiding this comment

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

One small change; otherwise good - feel free to merge once you've addressed it.

include/kllvm/ast/AST.h Outdated Show resolved Hide resolved
@rv-jenkins rv-jenkins merged commit 6757d77 into develop May 17, 2024
9 checks passed
@rv-jenkins rv-jenkins deleted the new-ordinal-att branch May 17, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants