riscv / sail-riscv

Sail RISC-V model
https://lists.riscv.org/g/tech-golden-model
Other
464 stars 168 forks source link

Add explicit vars and enable --strict-var #541

Closed Timmmm closed 2 months ago

Timmmm commented 2 months ago

This adds explicit var keywords where they were missing before, and enables the --strict-var flag to prevent us from missing them out in future. Implicit var was a mistake inherited from ASL.

Timmmm commented 2 months ago

Note, a lot of these vars are totally unnecessary and should be lets. In future we should go through the code and audit all vars and undefineds to make sure they are really appropriate.

@Alasdair the Sail compiler could probably benefit from a "var foo is never mutated; use let instead" warning. Or maybe even make it an error.

github-actions[bot] commented 2 months ago

Test Results

712 tests  ±0   712 :white_check_mark: ±0   0s :stopwatch: ±0s   6 suites ±0     0 :zzz: ±0    1 files   ±0     0 :x: ±0 

Results for commit 3d1c9ab1. ± Comparison against base commit ad8b8cb0.

:recycle: This comment has been updated with latest results.

Alasdair commented 2 months ago

I added the warning you suggested, but it looks like you fixed the place where it would occur already!

Warning: Unnecessary mutability model/riscv_fetch.sail:38.16-21:
38 |                PC_hi : xlenbits = PC + 2;
   |                ^---^
   | 
This variable is mutable, but it is never modified. It could be declared as immutable using 'let'.
Timmmm commented 2 months ago

Ha, I thought there would be more in the vector code but I guess those are all tuples where one part is modified.

Timmmm commented 2 months ago

@billmcspadden-riscv ok if I merge this?

billmcspadden-riscv commented 2 months ago

merged

On Tue, Sep 10, 2024 at 11:49 AM Tim Hutt @.***> wrote:

@billmcspadden-riscv https://github.com/billmcspadden-riscv ok if I merge this?

— Reply to this email directly, view it on GitHub https://github.com/riscv/sail-riscv/pull/541#issuecomment-2341767893, or unsubscribe https://github.com/notifications/unsubscribe-auth/AXROLOGDZQZN4H4FFMVOL4DZV45LRAVCNFSM6AAAAABN4BLABGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBRG43DOOBZGM . You are receiving this because you were mentioned.Message ID: @.***>

-- Bill McSpadden Formal Verification Engineer RISC-V International mobile: 503-807-9309

Join me at RISC-V Summit North America http://www.riscvsummit.com/ in October!

Alasdair commented 2 months ago

I think most of the vector code has the following shape:

var result = undefined;
var mask = undefined;

(result, mask) = init

If it was like var (result, mask) = init it would warn on mask being unmodified I think.

Timmmm commented 2 months ago

Ah cool, I'll go through and change them all at some point and then hopefully we'll see the warning.