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

Add support for arrayCopy, arraySet, and arrayRangeEqual primitives #1428

Merged
merged 5 commits into from
Aug 27, 2021

Conversation

andreistefanescu
Copy link
Contributor

Copy link
Contributor

@robdockins robdockins left a comment

Choose a reason for hiding this comment

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

Nitpicks about commenting arguments to the new primitives, but otherwise this seems like pretty straightforward plumbing.

I'm happy with this once upstream PRs land and CI gets worked out.

@@ -2242,6 +2245,18 @@ scArrayUpdate sc a b f i e = scGlobalApply sc "Prelude.arrayUpdate" [a, b, f, i,
scArrayEq :: SharedContext -> Term -> Term -> Term -> Term -> IO Term
scArrayEq sc a b x y = scGlobalApply sc "Prelude.arrayEq" [a, b, x, y]

-- > arrayCopy : (n : Nat) -> (a : sort 0) -> Array (Vec n Bool) a -> Vec n Bool -> Array (Vec n Bool) a -> Vec n Bool -> Vec n Bool -> Array (Vec n Bool) a;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's list the names of the arguments for these. There are a lot of them, and it's easy to swap around src/dest, etc.

@@ -184,6 +185,9 @@ prims =
, Prims.bpArrayLookup = unsupportedSBVPrimitive "bpArrayLookup"
, Prims.bpArrayUpdate = unsupportedSBVPrimitive "bpArrayUpdate"
, Prims.bpArrayEq = unsupportedSBVPrimitive "bpArrayEq"
, Prims.bpArrayCopy = unsupportedSBVPrimitive "bpArrayCopy"
, Prims.bpArraySet = unsupportedSBVPrimitive "bpArraySet"
, Prims.bpArrayRangeEq = unsupportedSBVPrimitive "bpArrayRangeEq"
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR wouldn't be the place to implement it, probably, but doesn't the SBV library have support for SMT arrays? Is there anything stopping us from supporting them?

Copy link
Contributor

Choose a reason for hiding this comment

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

As I recall, the "dynamic" SBV interface we use doesn't expose that functionality. I should double check to be sure, but I think that's why we don't support arrays or uninterpreted functions via Cryptol or SAW. It's one of the reasons I want to find a way to use the standard interface instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't remember exactly why I didn't implement array support in the SBV backend in the first place. I am not that familiar with SBV, if it's not that complicated we could add support in the future

sc_elm_type <- baseSCType sym sc range
SAWExpr <$> SC.scArrayEq sc sc_idx_type sc_elm_type x' y'
| otherwise ->
unsupported sym ("SAW backend: equality comparison on unsupported multidimensional array type:" ++ show tp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the reason not to support multi-dimensional arrays here that it would be extra work, in this function, that we don't need right now? Or are there limitations elsewhere in the codebase that would make it hard to support them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't support multi-dimensional arrays in cryptol or saw-core, and we don't need them at the moment. Adding representation for multi-dimensional arrays in cryptol and saw-core, and translations for all the round-tripping would add a lot of complexity, and I wanted to keep things simple

@andreistefanescu andreistefanescu merged commit 5035dcc into master Aug 27, 2021
@andreistefanescu andreistefanescu deleted the array-copy-set branch August 27, 2021 03:31
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