-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
New Rule: Implicit case default #281
Merged
dalance
merged 29 commits into
dalance:master
from
ronitnallagatla:implicit_case_default
Jun 3, 2024
Merged
Changes from all commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
47c3517
added explanation
ronitnallagatla bdd3831
added pass/fail examples
ronitnallagatla 1c0a7d0
started making changes to implicit_case_default.rs
ShreChinno 546873f
New Rule: implicit_case_default -- basic implementation
ronitnallagatla 68f609b
Commit from GitHub Actions (Run mdgen)
ronitnallagatla 1e9c772
Merge pull request #4 from ronitnallagatla/ronit
5han7anu-S 5073475
implicit_case_default works for most implicit declarations, still nee…
ShreChinno f445e1f
simple case implicit_case_default DONE
ShreChinno f665f23
Commit from GitHub Actions (Run mdgen)
ShreChinno ec897a6
added another fail testcase
ShreChinno b12a61f
Merge remote-tracking branch 'origin/shreyas' into shreyas
ShreChinno 8907da0
Commit from GitHub Actions (Run mdgen)
ShreChinno 977e4c3
removed debug prints
ShreChinno 78df364
small changes
ShreChinno 1a96bf0
Commit from GitHub Actions (Run mdgen)
ShreChinno 3d48239
changed hint
ShreChinno 611c74f
Commit from GitHub Actions (Run mdgen)
ShreChinno b3baa66
Merge pull request #5 from ronitnallagatla/shreyas
ronitnallagatla 57e4d5a
add locate on fail
ronitnallagatla fc18e12
add missing begin/end
ronitnallagatla 38f0769
Commit from GitHub Actions (Run mdgen)
ronitnallagatla 4bdb2c3
added more testcases
ronitnallagatla 67f6a4a
Commit from GitHub Actions (Run mdgen)
ronitnallagatla 7b753af
some cleanup/renaming
ronitnallagatla b3442e9
Merge pull request #6 from ronitnallagatla/ronit
ShreChinno 321c530
fixed line width in explanation
ronitnallagatla cf1215c
Commit from GitHub Actions (Run mdgen)
ronitnallagatla 9865291
implicit_case_default: updated explanation
ronitnallagatla ade2a24
Commit from GitHub Actions (Run mdgen)
ronitnallagatla File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
This rule is an extension of the **case_default** rule that allows the case | ||
default to be implicitly defined. Case statements without a `default` branch | ||
can cause signals to be undriven. Setting default values of signals at the top | ||
of an `always` procedures is good practice and ensures that signals are never | ||
metastable when a case match fails. For example, | ||
|
||
```sv | ||
always_comb begin | ||
y = 0; | ||
case (x) | ||
1: y = 1; | ||
endcase | ||
end | ||
|
||
``` | ||
|
||
If the case match on `x` fails, `y` would not infer memory or be undriven | ||
because the default value is defined before the `case`. | ||
|
||
This rule is a more lenient version of `case_default`. It adapts to a specific | ||
coding style of setting default values to signals at the top of a procedural | ||
block to ensure that signals have a default value regardless of the logic in the | ||
procedural block. As such, this rule will only consider values set | ||
**unconditionally** at the top of the procedural block as a default and will | ||
disregard assignments made in conditional blocks like `if`/`else`, etc. If this | ||
coding style is not preferred, it is strongly suggested to use the rules | ||
mentioned below as they offer stricter guarantees. | ||
|
||
See also: | ||
|
||
- **case_default** | ||
- **explicit_case_default** | ||
|
||
The most relevant clauses of IEEE1800-2017 are: | ||
|
||
- 12.5 Case statement |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
use crate::config::ConfigOption; | ||
use crate::linter::{SyntaxRule, SyntaxRuleResult}; | ||
use sv_parser::{unwrap_locate, unwrap_node, NodeEvent, RefNode, SyntaxTree}; | ||
|
||
#[derive(Default)] | ||
pub struct ImplicitCaseDefault { | ||
under_always_construct: bool, | ||
under_case_item: bool, | ||
has_default: bool, | ||
lhs_variables: Vec<String>, | ||
} | ||
|
||
impl SyntaxRule for ImplicitCaseDefault { | ||
fn check( | ||
&mut self, | ||
syntax_tree: &SyntaxTree, | ||
event: &NodeEvent, | ||
_option: &ConfigOption, | ||
) -> SyntaxRuleResult { | ||
let node = match event { | ||
NodeEvent::Enter(x) => { | ||
match x { | ||
RefNode::AlwaysConstruct(_) => { | ||
self.under_always_construct = true; | ||
self.has_default = false; | ||
} | ||
|
||
RefNode::CaseItemNondefault(_) => { | ||
self.under_case_item = true; | ||
} | ||
|
||
_ => (), | ||
} | ||
x | ||
} | ||
|
||
NodeEvent::Leave(x) => { | ||
match x { | ||
RefNode::AlwaysConstruct(_) => { | ||
self.under_always_construct = false; | ||
self.has_default = false; | ||
self.lhs_variables.clear(); | ||
} | ||
|
||
RefNode::CaseItemNondefault(_) => { | ||
self.under_case_item = false; | ||
} | ||
|
||
_ => (), | ||
} | ||
return SyntaxRuleResult::Pass; | ||
} | ||
}; | ||
|
||
// match implicit declarations | ||
if let (true, false, RefNode::BlockItemDeclaration(x)) = | ||
(self.under_always_construct, self.under_case_item, node) | ||
{ | ||
let var = unwrap_node!(*x, VariableDeclAssignment).unwrap(); | ||
let id = get_identifier(var, syntax_tree); | ||
self.lhs_variables.push(id); | ||
} | ||
|
||
// check if default | ||
if let (true, RefNode::CaseStatementNormal(x)) = (self.under_always_construct, node) { | ||
let a = unwrap_node!(*x, CaseItemDefault); | ||
if a.is_some() { | ||
self.has_default = true; | ||
} | ||
} | ||
|
||
// match case statement declarations | ||
match (self.under_always_construct, self.under_case_item, node) { | ||
(true, true, RefNode::BlockingAssignment(x)) => { | ||
let var = unwrap_node!(*x, VariableLvalueIdentifier).unwrap(); | ||
let loc = unwrap_locate!(var.clone()).unwrap(); | ||
let id = get_identifier(var, syntax_tree); | ||
|
||
if self.lhs_variables.contains(&id.to_string()) || self.has_default { | ||
return SyntaxRuleResult::Pass; | ||
} else { | ||
return SyntaxRuleResult::FailLocate(*loc); | ||
} | ||
} | ||
|
||
(true, true, RefNode::BlockItemDeclaration(x)) => { | ||
let var = unwrap_node!(*x, VariableDeclAssignment).unwrap(); | ||
let loc = unwrap_locate!(var.clone()).unwrap(); | ||
let id = get_identifier(var, syntax_tree); | ||
|
||
if self.lhs_variables.contains(&id.to_string()) || self.has_default { | ||
return SyntaxRuleResult::Pass; | ||
} else { | ||
return SyntaxRuleResult::FailLocate(*loc); | ||
} | ||
} | ||
|
||
_ => (), | ||
} | ||
|
||
SyntaxRuleResult::Pass | ||
} | ||
|
||
fn name(&self) -> String { | ||
String::from("implicit_case_default") | ||
} | ||
|
||
fn hint(&self, _option: &ConfigOption) -> String { | ||
String::from("Signal driven in `case` statement does not have a default value.") | ||
} | ||
|
||
fn reason(&self) -> String { | ||
String::from("Default values ensure that signals are never metastable.") | ||
} | ||
} | ||
|
||
fn get_identifier(node: RefNode, syntax_tree: &SyntaxTree) -> String { | ||
let id = match unwrap_node!(node, SimpleIdentifier, EscapedIdentifier) { | ||
Some(RefNode::SimpleIdentifier(x)) => Some(x.nodes.0), | ||
Some(RefNode::EscapedIdentifier(x)) => Some(x.nodes.0), | ||
_ => None, | ||
}; | ||
|
||
String::from(syntax_tree.get_str(&id).unwrap()) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
module M; | ||
always_comb | ||
case (x) | ||
1: a = 0; // No implicit or explicit case default | ||
endcase | ||
endmodule | ||
//////////////////////////////////////////////////////////////////////////////// | ||
module M; | ||
always_comb begin | ||
y = 0; | ||
case (x) | ||
1: y = 1; | ||
2: begin | ||
z = 1; | ||
w = 1; | ||
end | ||
endcase | ||
end | ||
endmodule | ||
//////////////////////////////////////////////////////////////////////////////// | ||
module M; | ||
always_comb begin | ||
a = 0; | ||
case (x) | ||
1: b = 0; | ||
endcase | ||
end | ||
endmodule |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@DaveMcEwan, thanks for your feedback. I have updated the explanation to indicate that this rule is not as strict as the alternatives. Is there anything else you might want to add or perhaps rephrase?