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

Lean: Manage when clauses in match_bv #1179

Closed
wants to merge 12 commits into from

Conversation

arthur-adjedj
Copy link
Contributor

This PR re-enables match_bv, and makes an attempt at managing when clauses correctly. This PR currently compiles RISC-V, but fails on a few C tests because of different limitations from the implementation:

  • encdec and encdec_subrange: The elaboration of bitvector pattern-matching inside another (non-bitvector) pattern such as here currently fails, and leads to ill-formed syntax.
  • option_nest and special_annot: some exhaustive patterns appear as let <pat> := <term>, but Lean does not manage to prove the exhaustiveness. A MWE of this issue is the following:
example : Bool :=
  /-missing cases:
    false-/
  match true with
    | true => true

This probably warrants a RFC on the Lean repo.

  • vector_init: The output differs from the expected output:
    Expected:
ok
ok
v[i] = 1
v[i] = 2
v[i] = 3
v[i] = 9223372036854775807000

Actual output (it's missing the first ok):

ok
v[i] = 1
v[i] = 2
v[i] = 3
v[i] = 9223372036854775807000
  • vector_subrange_pattern: some TODO_ARG_PATTERNs appear in a few places. My initial guess is that subrange patterns in function clauses are not deleted correctly by the remove_vector_subrange_pats rewrite pass, though I have not investigated this any further.

Furthermore, the make_cases_exhaustive rewrite pass does not work in the presence of patterns with when clauses, and failwiths when it encounters one, so I had to turn it off for now, leading to some matches getting an additional superfluous wildcard pattern-match here and there. It could probably be tweaked to simply give-up on the exhaustiveness checks whenever it encounters a when pattern.

I do not have more time to dedicate to this PR, so I will leave this here as is. Anyone feel free to try and fix these errors, or consider them to not matter too much and skip them in the test suite altogether.

tobiasgrosser and others added 10 commits March 18, 2025 20:17
The only semantic change this PR proposes is to not assert on
argument pattern that we cannot yet translate, but instead
to record them as `TODO_ARG_PATTERN`. This is sufficient to
build the full RISC-V Sail model without assertions and with
about 3500 warnings and errors remaining.

This PR also introduces a GitHub action to validate that the
RISC-V model builds. The RISC-V model currently requires two
patches (one in Ryan's repository and one applied through this patch).
Over time, we expect to switch to the main RISC-V repo.

Co-authored-by: Léo Stefanesco <[email protected]>
Copy link

github-actions bot commented Mar 19, 2025

Test Results

   13 files     27 suites   0s ⏱️
  854 tests   848 ✅ 0 💤 0 ❌ 6 🔥
3 716 runs  3 710 ✅ 0 💤 0 ❌ 6 🔥

For more details on these errors, see this check.

Results for commit 13bdc46.

♻️ This comment has been updated with latest results.

@arthur-adjedj arthur-adjedj changed the title Manage when clauses in match_bv Lean: Manage when clauses in match_bv Mar 19, 2025
@tobiasgrosser tobiasgrosser added the Lean Issues with Sail to Lean translation label Mar 19, 2025
@tobiasgrosser
Copy link
Collaborator

tobiasgrosser commented Mar 20, 2025

I just tested this. While the code looks great, I am getting 10 syntax and type errors on the full sail model and lean hangs at write_CSR when trying to build the full model. Here the code I am getting: https://github.com/opencompl/sail-riscv/pull/1/files

I currently get the error unexpected identifier; expected '_' with a line under reg:

def csr_name_map_forwards (arg_ : (BitVec 12)) : SailM String := do
  match_bv arg_ with
  | 001100000001 => do (pure "misa")
[...]
  | reg => do (hex_bits_12_forwards reg)

On the positive side, the bv_match model has 82000 lines vs the current model's 105000 lines.

@arthur-adjedj
Copy link
Contributor Author

I guess I did mess something up before putting up the PR then. Thanks for taking the time to run this on your end, I'll have it hopefully fixed soon.

@javra
Copy link
Collaborator

javra commented Mar 20, 2025

  match true with
    | true => true

Would we assume this to be exhaustive because the expression syntactically matches a constructor? I'm not sure I'd count on this being changed. OCaml also gives me a non-exhaustiveness warning on the same match block.

@javra
Copy link
Collaborator

javra commented Mar 20, 2025

vector_init: The output differs from the expected output:

That's to do with literals 🤔 In the second match block it matches an Option (BitVec 16) and states the value in binary, but prints it as 1111111111111111 instead of 0b1111111111111111. I'll try to fix it.

@Alasdair
Copy link
Collaborator

Sail also considers such a match statement as incomplete:

Warning: Incomplete pattern match statement at test.sail:8.2-7:
8 |  match true {
  |  ^---^
  | 
The following expression is unmatched: false

But it's just a warning in Sail not an error, so you might have to add _ => <raise error> case in the translated code.

@javra
Copy link
Collaborator

javra commented Mar 20, 2025

@arthur-adjedj: Pushed a fix for vector_init to javra:match_bv (can't push to your repo).

@javra
Copy link
Collaborator

javra commented Mar 20, 2025

One idea to sidestep the issues generally for now: We could add a pragma to Sail to mark functions in which match_bv should be used. We automatically mark those functions created by forward and backward matches of bidirectional mappings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Lean Issues with Sail to Lean translation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants