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

Fix compiler warnings in V extension code #415

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

ved-rivos
Copy link
Contributor

@ved-rivos ved-rivos commented Feb 25, 2024

  1. Handle unexpected match values as internal errors - they are checked dynamically.
  2. Remove deprecated set syntax

@jrtc27
Copy link
Collaborator

jrtc27 commented Feb 25, 2024

Should SEW’s type not instead be constrained to the set of valid values so you get static checking?

@ved-rivos
Copy link
Contributor Author

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.

@bacam
Copy link
Collaborator

bacam commented Feb 26, 2024

It would be better to follow the style of similar functions (e.g., VFUNARY1) which assert the restriction immediately after the check that should enforce it.

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).

@ved-rivos
Copy link
Contributor Author

It would be better to follow the style of similar functions (e.g., VFUNARY1) which assert the restriction immediately after the check that should enforce it.

That seems like a good idea. I will respin the PR with that fix.

@ved-rivos ved-rivos force-pushed the warning1 branch 2 times, most recently from 4e76d0b to 87792be Compare February 26, 2024 19:14
@ved-rivos
Copy link
Contributor Author

@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?

@bacam
Copy link
Collaborator

bacam commented Feb 27, 2024

That case is quite different. If you want to avoid using a wildcard, you could split rfvvfunct6 into separate types for the single-width and widening variants.

@ved-rivos
Copy link
Contributor Author

If you want to avoid using a wildcard, you could split rfvvfunct6 into separate types for the single-width and widening variants.

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?

@bacam
Copy link
Collaborator

bacam commented Feb 28, 2024

The type checker doesn't track enum values, so an assert won't suppress a incomplete pattern match warning.

@ved-rivos
Copy link
Contributor Author

Okay. Using a wildcard is not an issue if the type checker cannot prune this. This should address the various warnings.

Copy link
Collaborator

@Timmmm Timmmm left a 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.

@@ -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}
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

Copy link

github-actions bot commented Mar 25, 2024

Test Results

712 tests  ±0   712 ✅ ±0   0s ⏱️ ±0s
  6 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 9d53a1c. ± Comparison against base commit e187e02.

♻️ This comment has been updated with latest results.

@billmcspadden-riscv billmcspadden-riscv merged commit f1c043d into riscv:master Apr 15, 2024
2 checks passed
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.

6 participants