-
Notifications
You must be signed in to change notification settings - Fork 200
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
Add dff-name-style #1983
base: master
Are you sure you want to change the base?
Add dff-name-style #1983
Conversation
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #1983 +/- ##
==========================================
+ Coverage 92.85% 92.88% +0.02%
==========================================
Files 355 356 +1
Lines 26272 26373 +101
==========================================
+ Hits 24395 24496 +101
Misses 1877 1877
☔ View full report in Codecov by Sentry. |
Wow! Really fantastic! I looked through the tests you wrote, and they all look really good. Though I was hoping for the checks that are unimplemented: that I assume the performance wouldn't be terrible considering that this is a similar check to |
You're right! Let's see if #1912 lands. It introduces some helpers to handle blocking assignments that will be helpful. For those extra checks I would appreciate a bit of help as I'm not too experienced in Verilog. We should detect wrong types of assignments to variables considering their suffixes, but I'm not sure if I'm aware of every possibility. Using the following references and assuming "_q" and "_n" suffixes:
DFF Outputsdata_q = x; // Blocking assignment
assign data_q = x; // Continuous assignment
wire data_q = 1; // Net declaration assignment
data_q &= x; // Assign modify statements
data_q++; // Increment/decrement statements
DFF Inputsreg data_n = x; // Variable declaration assignment
data_n <= x; // Nonblocking assignment |
Hmm, I'm worried about trying to list all the combinational assignments. I feel like a better way is to:
The only way to create DFFs are with always_ff blocks. Everything else is combinational |
Also, non-blocking assignments are generally only valid inside always_ff blocks anyways. So I bet you can just continue using <= as your check for DFFs Thanks again! |
Also CC @fangism as he might be interested in this. What I can already say: there are a bunch of integration tests of lint rules in And, probably for a few of the other rules you have been working on recently, I might've forgotten to point that out then. |
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 wanted to say: incredible work so far!
// | ||
// Under this convention, data_q3 should be driven by the | ||
// previous stage data_q2 | ||
// |
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.
What about cases where LHS is a compound expression like:
{zz_q2, yy_q2} = ...
(It's ok to declare this as a TODO.)
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.
This would fail currently due to
that was basically added to ease the check. It is not clear to me how to deal with all the possibilities consistently.
Maybe I could add a comment saying something like: "Currently this rule only allows for simple identifiers, but it could be expanded to handle more complex references"
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.
Looks very nice. Haven't looked in full detail yet, so here just some first small nits.
Thanks for the reviews. I'll work on those integration tests. Separately, I worked a bit on the extra checks, but I'm not sure how to proceed as I added some of the needed helper already in #1912. Should I just add them here? I got away with a cheap trick for most of the cases as a temporary fix. Should this extra check be hidden under another configuration file? Added screenshot and diff for a quick overview. diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD
index 7fcfec0d..aeeabb07 100644
--- a/verilog/analysis/checkers/BUILD
+++ b/verilog/analysis/checkers/BUILD
@@ -2134,6 +2134,7 @@ cc_library(
"//common/analysis:syntax-tree-lint-rule",
"//common/analysis/matcher",
"//common/analysis/matcher:bound-symbol-manager",
+ "//common/analysis/matcher:core-matchers",
"//common/strings:naming-utils",
"//common/text:config-utils",
"//common/text:symbol",
diff --git a/verilog/analysis/checkers/dff_name_style_rule.cc b/verilog/analysis/checkers/dff_name_style_rule.cc
index ebcc757a..92d7b685 100644
--- a/verilog/analysis/checkers/dff_name_style_rule.cc
+++ b/verilog/analysis/checkers/dff_name_style_rule.cc
@@ -25,6 +25,7 @@
#include "absl/strings/string_view.h"
#include "common/analysis/lint_rule_status.h"
#include "common/analysis/matcher/bound_symbol_manager.h"
+#include "common/analysis/matcher/core_matchers.h"
#include "common/analysis/matcher/matcher.h"
#include "common/strings/naming_utils.h"
#include "common/text/config_utils.h"
@@ -76,8 +77,37 @@ static const Matcher &AlwaysFFMatcher() {
void DffNameStyleRule::HandleSymbol(const verible::Symbol &symbol,
const SyntaxTreeContext &context) {
verible::matcher::BoundSymbolManager manager;
- if (!AlwaysFFMatcher().Matches(symbol, &manager) ||
- (valid_input_suffixes.empty() && valid_output_suffixes.empty())) {
+ if (valid_input_suffixes.empty() && valid_output_suffixes.empty()) return;
+
+ // Types of assignments intended for DFF inputs
+ static const Matcher matcher = verible::matcher::AnyOf(
+ NodekContinuousAssignmentStatement(), NodekNetVariableAssignment(),
+ NodekNetDeclarationAssignment(), NodekAssignModifyStatement(),
+ NodekIncrementDecrementExpression());
+ if (matcher.Matches(symbol, &manager)) {
+ const verible::SyntaxTreeNode &node = SymbolCastToNode(symbol);
+ if (node.children().empty()) return;
+ const verible::Symbol *lhs = node.children().front().get();
+ absl::string_view lhs_str = verible::StringSpanOfSymbol(*lhs);
+
+ //TODO: helpers. Also, ++data_ff doesnt work (not 1st child)
+ const bool found_id = std::any_of(valid_output_suffixes.cbegin(),
+ valid_output_suffixes.cend(),
+ [&](const std::string &suffix) -> bool {
+ return lhs_str.size() > suffix.size() &&
+ absl::EndsWith(lhs_str, suffix);
+ });
+
+ if (!found_id) return;
+ violations_.insert(LintViolation(
+ *lhs,
+ absl::StrCat(lhs_str,
+ " should be driven by a nonblocking assignment "
+ "inside an always_ff block"),
+ context));
+ }
+
+ if (!AlwaysFFMatcher().Matches(symbol, &manager)) {
return;
} |
What is the last status here ? |
If I remember correctly everything was looking good. I had a local stash implementing some extra checks (basically, the diff I posted above) but that required some helpers also introduced here: #1912 I think the other PR will require more time to review and is less useful, so I could introduce the helpers here and later rebase that other PR and eventually get back at it. Therefore, I will:
EDIT: After I clean things up, I'll push and work on trying to support things like |
d7fb78f
to
c0e8f40
Compare
A couple of "minor" |
f2a2c75
to
e41c293
Compare
Thanks for all your work on this! I am excited for this to be merged. I found a few small issues, sorted by most to least importance. Params also flag the warning. Is it possible to only raise the warning on a non-constant expression? localparam DataResetValue = 4;
always_ff @(posedge clk_i) begin
if (!rst_ni) begin
data_q <= DataResetValue;
end else begin
data_q <= data_d;
end
end RAMs flag the warning, though this can realistically be corrected with a waiver. logic [7:0] MEM [256];
always_ff @(posedge clk_i) begin
if (we_i)
MEM[waddr_i] <= wdata_i;
end Nets with dff suffix should be only driven by always_ff block. Possibly not a big deal. logic data_q;
assign data_q = data_d; |
Thanks for providing feedback!
Sadly we can't know that
Sadly, same as above. The best thing we can do right now is inferring stuff from name formatting
This one should be flagged, could you double-check? |
Unfortunate, maybe this could be opened as another issue. I assume checking whether an expression is constant would be a useful metric in general. I don't think checking the capitalization of the label is a universal solution, but it could possibly be a good parameter of the rule. By the way, here are a few unique cases for resets that CVA6 uses:
LowRISC expects snake case for memories, so this doesn't seem like a good option. But it could potentially be another parameter?
My mistake, yes this is flagged. |
I agree it would be great, but I think it would require a lot of effort. It would be somewhat feasible to do for variables defined in the same
Thanks!
Yes, that's what I meant.
To summarize:
Thanks again for the feedback, and feel free to suggest more things. |
I think the current implementation is quite good and works well enough. In most RTL I've seen, reset just sets the net to I feel like the best long-term solution would be a check if an expression is constant, which could probably be put in another issue.
A memory regex sounds like a fantastic solution! With that logic, should the _d/_q names also use a regex? |
Okay. I'll keep in mind to try something to make the problem less annoying anyway, waiving is never fun.
I thought about it but I don't see much use for it in this particular rule as as we're always matching the suffix and we have to manually check the pipeline stage |
Add helpers to extract nonblocking assignments from a syntax tree. Add helpers to extract left and right hand sides from nonblocking assignments.
dff-name-style-rule adds support for enforcing naming conventions for registers. It takes two parameters in the form of a comma separated list of suffixes that are allowed for the input and output ports. If a list is empty, it means that no checks will be performed in the respective field. In addition to checking suffixes, the rule also enforces that the base (or prefix) is shared between the input and output of the register. Furthermore, it also allows for numbered identifiers, where the number means the pipeline stage where the signal originates from.
TODO: cleanup code, perhaps add some helpers around getting the parenthesis expression from an if TODO: parse the reset if string from configuration, currently hardcoded TODO: add regex for waiving memories Changes: - Remove original "waiving" for assigning some constants (data_q <= 0) note that it didn't work that well: (data_q <= 1'b0 failed) - Ignore nonblocking assignments inside a "waived condition". Basically, the idea is waiving reset assignments like: if(!rst_ni) begin data_q <= ... end
e41c293
to
92dd5f2
Compare
Addresses #1913 with some minor changes
Unimplemented
These kinds of checks are not implemented (yet?) although they should be fairly easy to add. My idea for it was:
valid_output_suffixes
verible/verilog/analysis/checkers/always_ff_non_blocking_rule.cc
Line 94 in a1cd07b
valid_input_suffixes
I wasn't sure it was worth the possible performance downside (!?) (maybe I'm overly pessimistic) I would add it if it is considered relevant.
Pinging @sifferman. Let me know if you have any suggestions/complaints. Sorry for the delay 😄