Skip to content

Commit

Permalink
fixup! Add autofixes for always_ff_non_blocking_rule
Browse files Browse the repository at this point in the history
  • Loading branch information
@IEncinas10
IEncinas10 committed May 21, 2023
1 parent de5d57b commit 6f5efe8
Show file tree
Hide file tree
Showing 3 changed files with 168 additions and 22 deletions.
120 changes: 109 additions & 11 deletions verilog/analysis/checkers/always_ff_non_blocking_rule.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <set>
#include <string>

#include "absl/strings/match.h"
#include "absl/strings/str_cat.h"
#include "absl/strings/string_view.h"
#include "common/analysis/lint_rule_status.h"
Expand Down Expand Up @@ -105,11 +106,13 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol,
// Rule may be waived if complete lhs consists of local variables
// -> determine root of lhs
const verible::Symbol *check_root = nullptr;
absl::string_view lhs_id;

verible::matcher::BoundSymbolManager symbol_man;
if (asgn_blocking_matcher.Matches(symbol, &symbol_man)) {
const verible::SyntaxTreeNode *node = &verible::SymbolCastToNode(symbol);
check_root = verilog::GetNetVariableAssignmentLhs(*node);
lhs_id = verible::StringSpanOfSymbol(*check_root);

const verible::SyntaxTreeLeaf *leaf =
verible::GetSubtreeAsLeaf(symbol, NodeEnum::kNetVariableAssignment, 1);
Expand All @@ -129,14 +132,16 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol,
const verible::SyntaxTreeNode &rhs = *GetAssignModifyRhs(*node);
const verible::SyntaxTreeLeaf &operator_ =
*GetAssignModifyOperator(*node);

check_root = &lhs;

const std::string fix =
absl::StrCat(verible::StringSpanOfSymbol(lhs),
" <= ", verible::StringSpanOfSymbol(lhs), " ",
verible::StringSpanOfSymbol(operator_).substr(0, 1), " ",
verible::StringSpanOfSymbol(rhs), ";");
bool needs_parenthesis = NeedsParenthesis(rhs);

lhs_id = verible::StringSpanOfSymbol(lhs);
const std::string fix = absl::StrCat(
lhs_id, " <= ", lhs_id, " ",
verible::StringSpanOfSymbol(operator_).substr(0, 1), " ",
needs_parenthesis ? "(" : "", verible::StringSpanOfSymbol(rhs),
needs_parenthesis ? ")" : "", ";");

autofixes.emplace_back(
AutoFix("Substitute assignment operator for equivalent "
Expand All @@ -149,17 +154,16 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol,
GetIncrementDecrementOperator(symbol);
check_root = operand;

const absl::string_view identifier =
verible::StringSpanOfSymbol(*operand);
lhs_id = verible::StringSpanOfSymbol(*operand);
// Extract just the operation. Just '+' from '++'
const absl::string_view op =
verible::StringSpanOfSymbol(*operator_).substr(0, 1);

// Equivalent nonblocking assignment
// {'x++', '++x'} become 'x <= x + 1'
// {'x--', '--x'} become 'x <= x - 1'
std::string fix =
absl::StrCat(identifier, " <= ", identifier, " ", op, " 1;");
const std::string fix =
absl::StrCat(lhs_id, " <= ", lhs_id, " ", op, " 1;");

autofixes.emplace_back(
AutoFix("Substitute increment/decrement operator for "
Expand Down Expand Up @@ -198,8 +202,12 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol,
}

// Enqueue detected violation unless waived
if (!waived) {
if (waived) return;

if (IsAutoFixSafe(symbol, lhs_id)) {
violations_.insert(LintViolation(symbol, kMessage, context, autofixes));
} else {
violations_.insert(LintViolation(symbol, kMessage, context));
}
} // HandleSymbol()

Expand All @@ -225,6 +233,7 @@ bool AlwaysFFNonBlockingRule::InsideBlock(const verible::Symbol &symbol,
if (always_ff_matcher.Matches(symbol, &symbol_man)) {
VLOG(4) << "always_ff @DEPTH=" << depth << std::endl;
inside_ = depth;
CollectLocalReferences(symbol);
}
return false;
}
Expand Down Expand Up @@ -267,5 +276,94 @@ bool AlwaysFFNonBlockingRule::LocalDeclaration(const verible::Symbol &symbol) {
return false;
} // LocalDeclaration()

bool AlwaysFFNonBlockingRule::NeedsParenthesis(
const verible::Symbol &rhs) const {
// Avoid inserting parenthesis for simple expressions
// For example: x &= 1 -> x <= x & 1, and not x <= x & (1)
// This could be more precise, but checking every specific
// case where parenthesis are needed is hard. Adding them
// doesn't hurt and the user can remove them if needed.
bool complex_rhs_expr =
verible::GetLeftmostLeaf(rhs) != verible::GetRightmostLeaf(rhs);

// Check whether the rhs expression is already inside parenthesis
// Avoid searching for kParenGroups, this should be safe and faster
return complex_rhs_expr &&
!absl::StrContains(verible::StringSpanOfSymbol(rhs), '(');
}

void AlwaysFFNonBlockingRule::CollectLocalReferences(
const verible::Symbol &root) {
static const Matcher reference_matcher{NodekReference()};

const std::vector<verible::TreeSearchMatch> references_ =
verible::SearchSyntaxTree(root, reference_matcher);

// Precompute the StringSpan of every identifier referenced to.
// Avoid recomputing it several times
references.resize(references_.size());
std::transform(references_.cbegin(), references_.cend(), references.begin(),
[](const verible::TreeSearchMatch &match) {
return ReferenceWithId{
match, verible::StringSpanOfSymbol(*match.match)};
});
}

bool AlwaysFFNonBlockingRule::IsAutoFixSafe(
const verible::Symbol &faulting_assignment,
absl::string_view lhs_id) const {
std::vector<ReferenceWithId>::const_iterator itr = references.end();

// Let's assume that 'x' is the variable affected by the faulting
// assignment. In order to ensure that the autofix is safe we have
// to ensure that there is no later reference to 'x'
//
// Can't autofix Can autofix
// begin begin
// x = x + 1; x = x + 1;
// y = x; y <= y + 1;
// end end
//
// In practical terms: we'll scan the 'references' vector for kReferences
// to 'x' that appear after the faulting assignment in the always_ff block

static const Matcher reference_matcher{NodekReference()};

// Extract kReferences inside the faulting expression
const std::vector<verible::TreeSearchMatch> references_ =
verible::SearchSyntaxTree(faulting_assignment, reference_matcher);

// ref points to the latest reference to 'x' in our faulting expression
// 'x++', 'x = x + 1', 'x &= x + 1'
// ^ ^ ^
// We need this to know from where to start searching for later references
// to 'x', and decide whether the AutoFix is safe or not
const verible::Symbol *ref =
std::find_if(std::rbegin(references_), std::rend(references_),
[&](const verible::TreeSearchMatch &m) {
return verible::StringSpanOfSymbol(*m.match) == lhs_id;
})
->match;

// Shouldn't happen, sanity check to avoid crashes
if (!ref) return false;

// Find the last reference to 'x' in the faulting assignment
itr = std::find_if(
std::begin(references), std::end(references),
[&](const ReferenceWithId &r) { return r.match.match == ref; });

// Search from the last reference to 'x' onwards. If there is any,
// we can't apply the autofix safely
itr = std::find_if(
std::next(itr), std::end(references),
[&](const ReferenceWithId &ref) { return ref.id == lhs_id; });

// Let's say 'x' is affected by a flagged operation { 'x = 1', 'x++', ... }
// We can safely autofix if after the flagged operation there are no
// more references to 'x'
return references.end() == itr;
}

} // namespace analysis
} // namespace verilog
34 changes: 27 additions & 7 deletions verilog/analysis/checkers/always_ff_non_blocking_rule.h
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2017-2020 The Verible Authors.
// Copyright 2017-2023 The Verible Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand All @@ -15,12 +15,12 @@
#ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_ALWAYS_FF_NON_BLOCKING_RULE_H_
#define VERIBLE_VERILOG_ANALYSIS_CHECKERS_ALWAYS_FF_NON_BLOCKING_RULE_H_

#include <set>
#include <stack>
#include <string>

#include "common/analysis/lint_rule_status.h"
#include "common/analysis/syntax_tree_lint_rule.h"
#include "common/analysis/syntax_tree_search.h"
#include "common/text/symbol.h"
#include "common/text/syntax_tree_context.h"
#include "verilog/analysis/descriptions.h"
Expand All @@ -32,19 +32,29 @@ class AlwaysFFNonBlockingRule : public verible::SyntaxTreeLintRule {
public:
using rule_type = verible::SyntaxTreeLintRule;

static const LintRuleDescriptor& GetDescriptor();
static const LintRuleDescriptor &GetDescriptor();

absl::Status Configure(absl::string_view configuration) final;
void HandleSymbol(const verible::Symbol& symbol,
const verible::SyntaxTreeContext& context) final;
void HandleSymbol(const verible::Symbol &symbol,
const verible::SyntaxTreeContext &context) final;

verible::LintRuleStatus Report() const final;

private:
// Detects entering and leaving relevant code inside always_ff
bool InsideBlock(const verible::Symbol& symbol, int depth);
bool InsideBlock(const verible::Symbol &symbol, int depth);
// Processes local declarations
bool LocalDeclaration(const verible::Symbol& symbol);
bool LocalDeclaration(const verible::Symbol &symbol);
// Check the need of parenthesis in certain autofixes
// Might have false positives. Example:
// Fixing 'x *= y + 1' requires adding parenthesis
// 'x <= x & (y + 1)'
bool NeedsParenthesis(const verible::Symbol &rhs) const;
// Collect local references inside an always_ff block
void CollectLocalReferences(const verible::Symbol &root);
// Check if is safe to autofix
bool IsAutoFixSafe(const verible::Symbol &faulting_assignment,
absl::string_view lhs_id) const;

private:
// Collected violations.
Expand All @@ -69,6 +79,16 @@ class AlwaysFFNonBlockingRule : public verible::SyntaxTreeLintRule {

// In-order stack of local variable names
std::vector<absl::string_view> locals_;

// NodekReference + string representation
// of the ID they're referring to
struct ReferenceWithId {
verible::TreeSearchMatch match;
absl::string_view id;
};
// Collection of references to variables
// assumed to be in program order
std::vector<ReferenceWithId> references;
};

} // namespace analysis
Expand Down
36 changes: 32 additions & 4 deletions verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,15 +140,11 @@ TEST(AlwaysFFNonBlockingRule, LocalWaiving) {
TEST(AlwaysFFNonBlockingRule, AutoFixDefault) {
const std::initializer_list<verible::AutoFixInOut> kTestCases = {
// Check we're not waiving local variables
{"module m;\nalways_ff begin reg k; k = 1; end\nendmodule",
"module m;\nalways_ff begin reg k; k <= 1; end\nendmodule"},
{"module m;\nalways_ff begin reg k; k = 1; end\nendmodule",
"module m;\nalways_ff begin reg k; k <= 1; end\nendmodule"},
// Check we're ignoring modifying assignments
{"module m;\nalways_ff begin k &= 1; k = 1; end\nendmodule",
"module m;\nalways_ff begin k &= 1; k <= 1; end\nendmodule"},
{"module m;\nalways_ff begin k &= 1; k = 1; end\nendmodule",
"module m;\nalways_ff begin k &= 1; k <= 1; end\nendmodule"},
};

RunApplyFixCases<VerilogAnalyzer, AlwaysFFNonBlockingRule>(kTestCases, "");
Expand All @@ -158,8 +154,40 @@ TEST(AlwaysFFNonBlockingRule, AutoFixCatchModifyingAssignments) {
const std::initializer_list<verible::AutoFixInOut> kTestCases = {
{"module m;\nalways_ff begin k &= 1; end\nendmodule",
"module m;\nalways_ff begin k <= k & 1; end\nendmodule"},
{"module m;\nalways_ff begin k &= a; end\nendmodule",
"module m;\nalways_ff begin k <= k & a; end\nendmodule"},
{"module m;\nalways_ff begin k |= (2 + 1); end\nendmodule",
"module m;\nalways_ff begin k <= k | (2 + 1); end\nendmodule"},
{"module m;\nalways_ff begin k *= 2 + 1; end\nendmodule",
"module m;\nalways_ff begin k <= k * (2 + 1); end\nendmodule"},
{"module m;\nalways_ff begin a++; end\nendmodule",
"module m;\nalways_ff begin a <= a + 1; end\nendmodule"},
{"module m;\nalways_ff begin ++a; end\nendmodule",
"module m;\nalways_ff begin a <= a + 1; end\nendmodule"},
};

RunApplyFixCases<VerilogAnalyzer, AlwaysFFNonBlockingRule>(
kTestCases, "catch_modifying_assignments:on");
}

TEST(AlwaysFFNonBlockingRule, AutoFixDontBreakCode) {
const std::initializer_list<verible::AutoFixInOut> kTestCases = {
// We can't fix 'k &= 1', because it would affect
// 'p <= k'
{"module m;\nalways_ff begin\n"
"k &= 1;\n"
"p <= k;\n"
"a++;"
"end\nendmodule",
"module m;\nalways_ff begin\n"
"k &= 1;\n"
"p <= k;\n"
"a <= a + 1;"
"end\nendmodule"},
// Correctly fix despide there is a reference to 'k' in the rhs
{"module m;\nalways_ff begin k = k + 1; end\nendmodule",
"module m;\nalways_ff begin k <= k + 1; end\nendmodule"},
// TODO: Add more tests
};

RunApplyFixCases<VerilogAnalyzer, AlwaysFFNonBlockingRule>(
Expand Down

0 comments on commit 6f5efe8

Please sign in to comment.