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: Adding features to support structs and bitfields #817

Merged
merged 23 commits into from
Jan 17, 2025

Conversation

lfrenot
Copy link
Collaborator

@lfrenot lfrenot commented Dec 6, 2024

This implements the translation of bitfields, and of the operations on structs (accessing a field, editing a field, making a new instance of a struct)

Copy link

github-actions bot commented Dec 6, 2024

Test Results

   12 files  ±0     24 suites  ±0   0s ⏱️ ±0s
  749 tests +1    749 ✅ +1  0 💤 ±0  0 ❌ ±0 
2 492 runs  +1  2 492 ✅ +1  0 💤 ±0  0 ❌ ±0 

Results for commit 8242d70. ± Comparison against base commit cf168b5.

♻️ This comment has been updated with latest results.

@javra
Copy link
Collaborator

javra commented Dec 16, 2024

Looks good! The convention for Lean files is to start with an upper case, I think we'd better stick to that

@lfrenot lfrenot force-pushed the lean-record-features branch from f15991b to 84df9be Compare December 17, 2024 13:26
@lfrenot lfrenot marked this pull request as ready for review December 17, 2024 13:35
@lfrenot lfrenot force-pushed the lean-record-features branch from b0de2a6 to 4583c5a Compare December 17, 2024 16:11
@lfrenot lfrenot force-pushed the lean-record-features branch from 3a9f237 to 9999aff Compare January 9, 2025 10:25
@lfrenot lfrenot force-pushed the lean-record-features branch from 67b5ae7 to e0ca20c Compare January 15, 2025 15:09
Comment on lines 18 to 23
def extractLsb {w: Nat} (x: BitVec w) (hi lo: Nat) : BitVec (hi - lo + 1) :=
x.extractLsb hi lo

Choose a reason for hiding this comment

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

Suggested change
def extractLsb {w: Nat} (x: BitVec w) (hi lo: Nat) : BitVec (hi - lo + 1) :=
x.extractLsb hi lo
def extractLsb {w : Nat} (x : BitVec w) (hi lo : Nat) : BitVec (hi - lo + 1) :=
x.extractLsb hi lo

Lean style puts a space before the : in arguments. I did notice the rest of the file has the same formatting, so we might want to address this in a separate formatting PR and fix the whole file in one go.

Comment on lines 15 to 16
def truncateLSB {w: Nat} (x: BitVec w) (w': Nat) : BitVec w' :=
x.extractLsb' 0 w'

Choose a reason for hiding this comment

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

Suggested change
def truncateLSB {w: Nat} (x: BitVec w) (w': Nat) : BitVec w' :=
x.extractLsb' 0 w'
def truncateLsb {w: Nat} (x: BitVec w) (w': Nat) : BitVec w' :=
x.extractLsb' 0 w'

We generally treat Lsb as one word, for the sake of capitalization. (Outside of the diff of this PR, so feel free to ignore)

Comment on lines 21 to 27
def update_subrange' {w: Nat} (x: BitVec w) (start len: Nat) (y: BitVec len) : BitVec w :=
let mask := ~~~(((BitVec.allOnes len).zeroExtend w) <<< start)
let y' := mask ||| ((y.zeroExtend w) <<< start)
x &&& y'

def update_subrange {w: Nat} (x: BitVec w) (hi lo: Nat) (y: BitVec (hi - lo + 1)) : BitVec w :=
update_subrange' x lo _ y

Choose a reason for hiding this comment

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

Suggested change
def update_subrange' {w: Nat} (x: BitVec w) (start len: Nat) (y: BitVec len) : BitVec w :=
let mask := ~~~(((BitVec.allOnes len).zeroExtend w) <<< start)
let y' := mask ||| ((y.zeroExtend w) <<< start)
x &&& y'
def update_subrange {w: Nat} (x: BitVec w) (hi lo: Nat) (y: BitVec (hi - lo + 1)) : BitVec w :=
update_subrange' x lo _ y
def updateSubrange' {w: Nat} (x: BitVec w) (start len: Nat) (y: BitVec len) : BitVec w :=
let mask := ~~~(((BitVec.allOnes len).zeroExtend w) <<< start)
let y' := mask ||| ((y.zeroExtend w) <<< start)
x &&& y'
def updateSubrange {w: Nat} (x: BitVec w) (hi lo: Nat) (y: BitVec (hi - lo + 1)) : BitVec w :=
updateSubrange' x lo _ y

Lean style uses lowerCamelCase for definitions

end BitVec
end Sail

structure registerRef (regstate regval a : Type) where

Choose a reason for hiding this comment

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

Suggested change
structure registerRef (regstate regval a : Type) where
structure RegisterRef (regstate regval a : Type) where

In this case, RegisterRef is a type(-valued function) and thus is written UpperCamelCase! (Sorry for not catching that before)

Comment on lines 12 to 16
def truncate {w: Nat} (x: BitVec w) (w': Nat) : BitVec w' :=
def truncate {w : Nat} (x : BitVec w) (w' : Nat) : BitVec w' :=
x.truncate w'

def truncateLSB {w: Nat} (x: BitVec w) (w': Nat) : BitVec w' :=
def truncateLsb {w : Nat} (x : BitVec w) (w' : Nat) : BitVec w' :=
x.extractLsb' 0 w'

Choose a reason for hiding this comment

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

Again, this is stuff outside the scope of your PR diff, but since you're cleaning it up I'll comment anyway:
truncate and truncateLsb are provably equivalent. It's unclear to me why we need both definitions

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, let's delete truncateLsb and avoid redundant defs.

Thank you for the annotations, @alexkeizer!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I see
This is my bad, truncateLsb is supposed to extract the most significant bits, the first argument is wrong here.
Since this changes more, I think I'll just open a separate PR to fix this and the formatting

@lfrenot
Copy link
Collaborator Author

lfrenot commented Jan 16, 2025

This will fail until #870 is merged, because I need the parentheses fix it introduces

@lfrenot lfrenot changed the title Lean: Adding features to support bitfields Lean: Adding features to support structs and bitfields Jan 16, 2025
@lfrenot lfrenot force-pushed the lean-record-features branch from 662fa50 to ab6fefd Compare January 16, 2025 14:24

/-- Type quantifiers: i : Int -/
def mk_struct (i : Int) (b : (BitVec 1)) : My_struct :=
{field1 := i, field2 := b}
Copy link
Collaborator

@javra javra Jan 16, 2025

Choose a reason for hiding this comment

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

Canonical Lean style would mandate spaces inside the curly brackets. But we could also print this as

def mk_struct (i : Int) (b : (BitVec 1)) : My_struct :=
  { field1 := i
    field2 := b }

which might be better for bigger terms

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree with your solution, I'll do that

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Note that we don't even need the , if we do the indentation right)

Copy link
Collaborator Author

@lfrenot lfrenot Jan 16, 2025

Choose a reason for hiding this comment

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

To be sure I follow the style: should it be

def mk_struct (i : Int) (b : (BitVec 1)) : My_struct :=
  {
    field1 := i
    field2 := b
  }

or

def mk_struct (i : Int) (b : (BitVec 1)) : My_struct :=
  { field1 := i
    field2 := b }

Copy link
Collaborator

Choose a reason for hiding this comment

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

The latter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Choose a reason for hiding this comment

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

Note that the following should also be a valid spelling of the same definition:

def mk_struct (i : Int) (b : (BitVec 1)) : My_struct where
  field1 := i
  field2 := b

Not that there's any clear benefit over this style, so I'm not suggesting we switch to it necessarily, just wanted to make you aware of the full space of options

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This wouldn't be practical here I think, since the := is in the function definition, not in the expression.
but good to know

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I think keeping the := is easier. This might be a task that could be given to the mighty future Lean reformatter ™

@lfrenot lfrenot force-pushed the lean-record-features branch from 523c16e to 8242d70 Compare January 16, 2025 16:10
@bacam bacam merged commit f87c292 into rems-project:sail2 Jan 17, 2025
3 checks passed
@lfrenot lfrenot deleted the lean-record-features branch January 17, 2025 14:32
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.

4 participants