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

Rangemap #3461

Merged
merged 17 commits into from
Jun 13, 2023
Merged

Rangemap #3461

merged 17 commits into from
Jun 13, 2023

Conversation

mariaKt
Copy link
Contributor

@mariaKt mariaKt commented Jun 12, 2023

This PR adds support for a new builtin type, range map, in K. Previous PRs have introduced the new type (here) and added hooks in the LLVM backend (here).

The bulk of the changes are in domains.md, defining the relevant module.

@rv-jenkins rv-jenkins changed the base branch from master to develop June 12, 2023 16:00
@mariaKt mariaKt marked this pull request as ready for review June 12, 2023 17:38
@mariaKt mariaKt requested review from radumereuta, Baltoli and dwightguth and removed request for dwightguth and ehildenb June 12, 2023 17:38
implementation of range maps provided by the backend.
Although the underlying range map data structure supports any key sort, the
current implementation by the backend only supports `Int` keys due to
limitations of the underlying ordering function.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supported by the haskell backend as well? If not, then I think it should be mentioned here that only the llvm backend implements this feature.

I think we also have some module attributes to protect against wrongful inclusion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In a follow-up PR, we can probably make an inefficient version of the RangeMap sort written entirely in K for use by the Haskell backend. I don't see the need to adjust the module attributes currently, although I agree about the documentation.

@ehildenb
Copy link
Member

Hmmmm, what's the use case here? It seems to me that it's probably mostly useful for modelling byte-arrays, but I think the #buf abstraction in KEVM does that better.

Or perhaps the idea here is a general notion of Array datatype? In that case, I think we should probably make an abstract Array instead of the AC rangemap.

How is this datastructure being used?

implementation of range maps provided by the backend.
Although the underlying range map data structure supports any key sort, the
current implementation by the backend only supports `Int` keys due to
limitations of the underlying ordering function.
Copy link
Collaborator

Choose a reason for hiding this comment

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

In a follow-up PR, we can probably make an inefficient version of the RangeMap sort written entirely in K for use by the Haskell backend. I don't see the need to adjust the module attributes currently, although I agree about the documentation.

@dwightguth
Copy link
Collaborator

dwightguth commented Jun 12, 2023

The use case is fairly straightforward. You want to be able to map ranges of integers to values and then look up the value associated with a particular integer. Neither an array nor #buf does this in a fashion that would allow you to perform both operations in log time and linear space in the number of ranges. We're talking about cases where there might be hundreds of thousands of contiguous integers that all map to the exact same value spread out across an incredibly sparse larger range of integers... Neither an array nor any existing data type in K can do this without using vastly more memory than is practical.

@mariaKt
Copy link
Contributor Author

mariaKt commented Jun 12, 2023

@ehildenb Specifically, we are dealing with this when using the C semantics with programs containing large static arrays. Currently, we are using the traditional K map to represent the memory cell, which is proving to be inefficient with such programs.

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.

A couple of small comments but this looks really good @mariaKt - I have tried to review tests from each group, and what's there is great, but as there are ~100 tests it's possible I missed something.

k-distribution/include/kframework/builtin/domains.md Outdated Show resolved Hide resolved

### Implementation of range maps

TBD
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to merge this into domains.md, we should have a more descriptive comment here explaining why the implementation is TBD.

@dwightguth dwightguth merged commit d766261 into develop Jun 13, 2023
@dwightguth dwightguth deleted the rangemap branch June 13, 2023 20:01
h0nzZik pushed a commit to h0nzZik/k that referenced this pull request Jun 19, 2023
…untimeverification#3134)

* haskell-backend/src/main/native/haskell-backend: 8612bdfe4 - Remove redundant fields from RPC timeout response (runtimeverification#3474)

* Sync flake inputs to submodules

* haskell-backend/src/main/native/haskell-backend: 4e9b76ab1 - Update README section (runtimeverification#3473)

* Sync flake inputs to submodules

* haskell-backend/src/main/native/haskell-backend: fae73ac06 - Switch to GHC 9.2.5 (runtimeverification#3461)

* Sync flake inputs to submodules

* haskell-backend/src/main/native/haskell-backend: f3e87176a - [runtimeverification#2313] Remove bmc code (runtimeverification#3482)

* Sync flake inputs to submodules

* Remove kbmc tests

---------

Co-authored-by: rv-jenkins <[email protected]>
Co-authored-by: ana-pantilie <[email protected]>
@Baltoli Baltoli mentioned this pull request Dec 12, 2023
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.

5 participants