-
Notifications
You must be signed in to change notification settings - Fork 164
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
Fix compiler warnings in V extension code #415
Conversation
ved-rivos
commented
Feb 25, 2024
•
edited
Loading
edited
- Handle unexpected match values as internal errors - they are checked dynamically.
- Remove deprecated set syntax
Should SEW’s type not instead be constrained to the set of valid values so you get static checking? |
They are. But all encoding's are not legal for all ops. |
It would be better to follow the style of similar functions (e.g., As @jrtc27 said, ideally we would have the type system enforce this statically. I have experimented a little with this, but some limitations in Sail's type system make it a little too awkward to submit at the moment. It did manage to detect a problem, though (#374). |
That seems like a good idea. I will respin the PR with that fix. |
4e76d0b
to
87792be
Compare
@bacam - I put the asserts in place. I cant seem to use an assert to mute the warning on the match case in riscv_insts_vext_red.sail . Suggestions? |
That case is quite different. If you want to avoid using a wildcard, you could split |
Using a wildcard is not an issue. i was however expecting the assert to prune those paths since rfvvfunct6 is a enum. I guess then such pruning using assets only works for sets? |
The type checker doesn't track enum values, so an assert won't suppress a incomplete pattern match warning. |
Okay. Using a wildcard is not an issue if the type checker cannot prune this. This should address the various warnings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than the minor comment.
model/riscv_insts_vext_red.sail
Outdated
@@ -185,7 +185,8 @@ function process_rfvv_single(funct6, vm, vs2, vs1, vd, num_elem_vs, SEW, LMUL_po | |||
FVV_VFREDOSUM => fp_add(rm_3b, sum, vs2_val[i]), | |||
FVV_VFREDUSUM => fp_add(rm_3b, sum, vs2_val[i]), | |||
FVV_VFREDMAX => fp_max(sum, vs2_val[i]), | |||
FVV_VFREDMIN => fp_min(sum, vs2_val[i]) | |||
FVV_VFREDMIN => fp_min(sum, vs2_val[i]), | |||
_ => {internal_error(__FILE__, __LINE__, "Widening op unexpected"); sum} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove sum
and just do
_ => internal_error(__FILE__, __LINE__, "Widening op unexpected"),
internal_error
is divergent so its return type isn't checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.