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

[slang] Add an inputs/edges combination coverage check for udp #1087

Merged
merged 6 commits into from
Oct 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/slang/ast/symbols/MemberSymbols.h
Original file line number Diff line number Diff line change
Expand Up @@ -261,12 +261,14 @@ class SLANG_EXPORT PrimitiveSymbol : public Symbol, public Scope {
std::string_view inputs;
char state = 0;
char output = 0;
bool isEdgeSensitive = false;
};

std::span<const PrimitivePortSymbol* const> ports;
std::span<const TableEntry> table;
const ConstantValue* initVal = nullptr;
bool isSequential = false;
bool isEdgeSensitive = false;
enum PrimitiveKind { UserDefined, Fixed, NInput, NOutput, BiDiSwitch } primitiveKind;

PrimitiveSymbol(Compilation& compilation, std::string_view name, SourceLocation loc,
Expand Down
4 changes: 3 additions & 1 deletion scripts/diagnostics.txt
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,8 @@ warning warning-task WarningTask "$warning encountered{}"
note InfoTask "$info encountered{}"
note NoteComparisonReduces "comparison reduces to ({} {} {})"
note NoteCommonAncestor "common ancestor here violates that constraint"
warning udp-coverage UdpCoverage "if the behavior of the UDP is sensitive to edges of any input, the desired output state shall be specified for all edges of all inputs."
note NoteUdpCoverage "missed desired output for rows:\n{}"
warning explicit-static StaticInitializerMustBeExplicit "initializing a static variable in a procedural context requires an explicit 'static' keyword"
warning case-gen-dup CaseGenerateDup "more than one case generate block matches the value {}"
warning case-gen-none CaseGenerateNoBlock "no case generate expression matches the value {}"
Expand Down Expand Up @@ -1147,7 +1149,7 @@ group extra = { empty-member empty-stmt dup-import pointless-void-cast case-gen-
float-narrow }

group pedantic = { empty-pattern implicit-net-port nonstandard-escape-code nonstandard-generate format-multibit-strength
nonstandard-sys-func nonstandard-foreach nonstandard-dist isunbounded-param-arg }
nonstandard-sys-func nonstandard-foreach nonstandard-dist isunbounded-param-arg udp-coverage }

group conversion = { width-trunc width-expand port-width-trunc port-width-expand implicit-conv
constant-conversion sign-conversion float-bool-conv int-bool-conv float-int-conv
Expand Down
26 changes: 26 additions & 0 deletions scripts/warning_docs.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1353,3 +1353,29 @@ module m;
end
endmodule
```

-Wudp-coverage
Checks if the behavior of the UDP is sensitive to edges of any input,
the desired output state shall be specified for all edges of all inputs.
That constraint is specified by LRM 29.6 section and it is not clear about it.
This can be a useful for checking edge-sensitive UDP missed transitions
when developing and simulating a behavioral model of industrial cells
which commonly implemented as verilog primitives.
```
primitive d_edge_ff (q, clock, data);
output q; reg q;
input clock, data;
table
// clock data q q+
// obtain output on rising edge of clock
(01) 0 : ? : 0 ;
(01) 1 : ? : 1 ;
(0?) 1 : 1 : 1 ;
(0?) 0 : 0 : 0 ;
// ignore negative edge of clock
(?0) ? : ? : - ;
// missed clock edge (1x) specification
// missed edge specifications for data signal
endtable
endprimitive
```
212 changes: 198 additions & 14 deletions source/ast/symbols/MemberSymbols.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -766,23 +766,16 @@ void PrimitivePortSymbol::serializeTo(ASTSerializer& serializer) const {
// Each 'bit' is one input value in the row.
class BitTrie {
public:
// Tries to insert the row. Returns nullopt if the row is
// successfully inserted, and a set of the previously
// inserted rows with the same bit pattern if a collision
// is found.
template<typename TAllocator>
std::optional<SmallVector<const UdpEntrySyntax*, 4>> insert(const UdpEntrySyntax& syntax,
std::span<const char> inputs,
char stateChar,
TAllocator& allocator) {
void find(BitTrie*& primaryNode, std::span<const char> inputs, char stateChar,
TAllocator& allocator, SmallVector<const UdpEntrySyntax*, 4>& result) {
SmallVector<BitTrie*> nodes;
BitTrie* primary = this;
nodes.push_back(this);

auto advance = [&](char c) {
SmallVector<BitTrie*> nextNodes;
for (auto node : nodes)
node->nextNodesFor(c, nextNodes, primary, allocator);
node->nextNodesFor(c, nextNodes, primaryNode, allocator);
nodes = std::move(nextNodes);
};

Expand Down Expand Up @@ -828,12 +821,24 @@ class BitTrie {

// If any of our nodes have entries we won't insert,
// as it means we have a duplicate.
SmallVector<const UdpEntrySyntax*, 4> eSyntaxes;
for (auto node : nodes) {
if (node->entry)
eSyntaxes.push_back(node->entry);
result.push_back(node->entry);
}
}

// Tries to insert the row. Returns nullopt if the row is
// successfully inserted, and a set of the previously
// inserted rows with the same bit pattern if a collision
// is found.
template<typename TAllocator>
std::optional<SmallVector<const UdpEntrySyntax*, 4>> insert(const UdpEntrySyntax& syntax,
std::span<const char> inputs,
char stateChar,
TAllocator& allocator) {
BitTrie* primary = this;
SmallVector<const UdpEntrySyntax*, 4> eSyntaxes;
find(primary, inputs, stateChar, allocator, eSyntaxes);
// Always store so as not to miss info it in case of possible overlap.
// If the primary->entry already has a value,
// then rewriting will not spoil anything,
Expand Down Expand Up @@ -948,8 +953,10 @@ static void createTableRow(const Scope& scope, const UdpEntrySyntax& syntax,
SmallVector<char> inputs;
size_t numInputs = 0;
bool allX = true;
bool isEdgeSensitive = false;
for (auto input : syntax.inputs) {
if (input->kind == SyntaxKind::UdpEdgeField) {
isEdgeSensitive = true;
auto& edge = input->as<UdpEdgeFieldSyntax>();
inputs.push_back('(');

Expand Down Expand Up @@ -989,6 +996,18 @@ static void createTableRow(const Scope& scope, const UdpEntrySyntax& syntax,
auto tok = input->as<UdpSimpleFieldSyntax>().field;
for (auto c : tok.rawText()) {
auto d = charToLower(c);
switch (d) {
case '*':
case 'r':
case 'f':
case 'p':
case 'n':
isEdgeSensitive = true;
break;
default:
break;
}

inputs.push_back(d);
numInputs++;
allX &= d == 'x';
Expand Down Expand Up @@ -1094,7 +1113,8 @@ static void createTableRow(const Scope& scope, const UdpEntrySyntax& syntax,
}

auto inputSpan = inputs.copy(scope.getCompilation());
table.push_back({std::string_view(inputSpan.data(), inputSpan.size()), stateChar, outputChar});
table.push_back({std::string_view(inputSpan.data(), inputSpan.size()), stateChar, outputChar,
isEdgeSensitive});
}

PrimitiveSymbol& PrimitiveSymbol::fromSyntax(const Scope& scope,
Expand Down Expand Up @@ -1337,10 +1357,174 @@ PrimitiveSymbol& PrimitiveSymbol::fromSyntax(const Scope& scope,
BumpAllocator alloc;
PoolAllocator<BitTrie> trieAlloc(alloc);
SmallVector<TableEntry> table;
for (auto entry : syntax.body->entries)
for (auto entry : syntax.body->entries) {
createTableRow(scope, *entry, table, ports.size(), trie, trieAlloc);
if (!table.empty() && !prim->isEdgeSensitive)
prim->isEdgeSensitive = table.back().isEdgeSensitive;
}

prim->table = table.copy(comp);

// Check that if the behavior of the UDP is sensitive to edges of any input, the desired
// output state shall be specified for all edges of all inputs.
if (prim->isEdgeSensitive && !prim->table.empty()) {
auto portSize = ports.size();
auto numInput = (portSize >= 2) ? portSize - 1 : portSize;
// Ordered list of all possible clock edges
const static std::vector<std::string_view> edgeScope = {"(01)", "(0x)", "(10)",
"(1x)", "(x0)", "(x1)"};
// Init state is (??)000...
std::string_view initEdge = "(??"
")";
std::vector<char> initialState(initEdge.begin(), initEdge.end());
initialState.insert(initialState.end(), numInput - 1, '0');
std::vector<char> state = initialState;
// End state is xxx...(x1)
std::string_view endEdge = "(x1)";
std::vector<char> endState(numInput - 1, 'x');
endState.insert(endState.end(), endEdge.begin(), endEdge.end());
// Iterate state by increase leftoutermost inputs.
// For simple inputs value '0' goes to '1' and value '1' goes to `x`.
// For edge inputs circular iteration occurs over 'edgeScope' list.
auto incrementState = [&initEdge](std::vector<char>& state) {
for (unsigned i = 0; i < state.size(); ++i) {
// Increase edge input
if (state.at(i) == '(') {
std::string currEdge{state.at(i), state.at(i + 1), state.at(i + 2),
state.at(i + 3)};
if (currEdge == edgeScope.back()) {
if (state.back() != 'x') {
if (i != 0) {
std::copy(edgeScope[0].begin(), edgeScope[0].end(),
state.begin() + i);
}
else {
// If it is edge input at first position set it as '(??)'
// instead of '(01)' for heuristically check it and skip any
// other edge inputs if '(??)' is present.
std::copy(initEdge.begin(), initEdge.end(), state.begin());
}

i += 3;
continue;
}
else {
// Move edge input to the next position
// and set first input as '?' and all other as '0'
state.at(0) = '?';
for (unsigned j = 1; j < state.size(); ++j)
state.at(j) = '0';

std::copy(edgeScope[0].begin(), edgeScope[0].end(),
state.begin() + i + 1);
break;
}
}
else {
size_t nextPos =
(state.at(i + 1) != '?')
? (size_t)std::distance(edgeScope.begin(),
std::find(edgeScope.begin(),
edgeScope.end(), currEdge)) +
1
: 0U;
std::copy(edgeScope[nextPos].begin(), edgeScope[nextPos].end(),
state.begin() + i);
break;
}
}

if (state.at(i) == '0') {
state.at(i) = '1';
break;
}

if (state.at(i) == '1') {
state.at(i) = 'x';
break;
}

if (i == 0) {
// If it is simple input at first position set it as '?'
// instead of '0' for heuristically check it and skip any
// other inputs if '?' is present.

if (state.at(i) == '?') {
state.at(i) = '0';
break;
}

if (state.at(i) == 'x')
state.at(i) = '?';
continue;
}

state.at(i) = '0';
}

return state.at(0) == '?' || state.at(1) == '?';
};

Diagnostic* diag = nullptr;
SmallVector<std::string, 8> missingRows;
bool isVariable = true;
// Try to find the missing combinations by completely enumerating all possible table
// rows
bool next = true;
while (next) {
// Comparing current state with end state
if (state.back() == ')' && (numInput == 1 || state[0] == 'x'))
next = (state != endState);

BitTrie* triePtr = &trie;
SmallVector<const UdpEntrySyntax*, 4> results;
// Try to find a desired input in an existing table structure
trie.find(triePtr, state, '?', trieAlloc, results);
if (results.empty()) {
if (!diag)
diag = &scope.addDiag(diag::UdpCoverage, prim->location);

std::string noteStr;
bool nextSplit = true;
for (auto c : state) {
if (c == '(')
nextSplit = false;
else if (c == ')')
nextSplit = true;

noteStr += c;
if (nextSplit)
noteStr += ' ';
}
missingRows.push_back(noteStr);

bool wasVariable = isVariable;
// If state with first any value input ('(??)' for edge input or '?' for simple
// input) was not found then we don't need to check any other state with
// explicit clock edges. So skip the state until we get a new any value clock
// edge state.
do {
isVariable = incrementState(state);
if (state.back() == ')' && (numInput == 1 || state[0] == 'x'))
next = (state != endState);
} while (next && wasVariable && !isVariable);

continue;
}

isVariable = incrementState(state);
}

if (diag && !missingRows.empty()) {
std::string noteStr;
for (auto& missingRow : missingRows) {
noteStr += {missingRow.data(), missingRow.size()};
noteStr += "\n";
}

diag->addNote(diag::NoteUdpCoverage, prim->location) << noteStr;
}
}
}

prim->ports = ports.copy(comp);
Expand Down
Loading