Skip to content

Commit

Permalink
Fix inline blacklist logic for recursive case
Browse files Browse the repository at this point in the history
Before if we had a module such as:
```verilog
module test_module (
    input [4:0] i1,
    input [4:0] i2,
    output [3:0] o0,
    output [3:0] o1,
    output [3:0] o2
);
wire [4:0] x;
wire [4:0] y;
wire [4:0] h;
wire [4:0] g;
assign x = i1 + i2;
assign h = i1 - i2;
assign g = h;
assign o0 = x[3:0];
assign y = i1;
assign o1 = y[3:0];
assign o2 = g[3:0];
endmodule;
```

We would get

```verilog
module test_module (
    input [4:0] i1,
    input [4:0] i2,
    output [3:0] o0,
    output [3:0] o1,
    output [3:0] o2
);
wire [4:0] x;
wire [4:0] h;
assign x = i1 + i2;
assign o0 = x[3:0];
assign o1 = i1[3:0];
assign o2 = (i1 - i2)[3:0];
endmodule;
```

Notice the (i1 - i2) is inlined into the slice node, which is invalid.

The reason was the blacklisting logic was not recursively checking
identifiers.  That is, when we encounter a slice, we let a an
identifier be inlined for another identifier into the contents of the
slice.  However, this is problematic when the identifier being inlined
will in turn be replaced by something which may not be valid (e.g. an
expression).  So, we improve the blacklisting logic to recursively check
inlined identifiers until we encounter an invalid driver of inlining.
At this point, we blacklist the current identifier so that it will not
be eventually inlined into the slice/index node.

So, for the above example we now get
```verilog
module test_module (
    input [4:0] i1,
    input [4:0] i2,
    output [3:0] o0,
    output [3:0] o1,
    output [3:0] o2
);
wire [4:0] x;
wire [4:0] h;
assign x = i1 + i2;
assign h = i1 - i2;
assign o0 = x[3:0];
assign o1 = i1[3:0];
assign o2 = h[3:0];
endmodule;
```
  • Loading branch information
leonardt committed Aug 27, 2020
1 parent eaa5a43 commit 0aa2569
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 18 deletions.
1 change: 1 addition & 0 deletions include/verilogAST/assign_inliner.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ class Blacklister : public Transformer {

protected:
bool blacklist = false;
void blacklist_invalid_driver(std::unique_ptr<Identifier> node);

public:
Blacklister(std::set<std::string> &wire_blacklist,
Expand Down
31 changes: 22 additions & 9 deletions src/assign_inliner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,31 @@

namespace verilogAST {

void Blacklister::blacklist_invalid_driver(std::unique_ptr<Identifier> node) {
if (!assign_map.count(node->toString())) {
// Not in assign map, means it's a module input, don't need to do anything
// because it won't be inlined
return;
}
auto driver = assign_map[node->toString()]->clone();
// Can only inline if driven by identifier, index, or slice
bool valid_driver = dynamic_cast<Identifier*>(driver.get()) ||
dynamic_cast<Index*>(driver.get()) ||
dynamic_cast<Slice*>(driver.get());
if (!valid_driver) {
this->wire_blacklist.insert(node->value);
} else if (auto ptr = dynamic_cast<Identifier*>(driver.get())) {
// if driven by an id, we need to recursively blacklist any invalid
// drivers, else they'll eventually get inlined into here
driver.release();
blacklist_invalid_driver(std::unique_ptr<Identifier>(ptr));
}
}

std::unique_ptr<Identifier> Blacklister::visit(
std::unique_ptr<Identifier> node) {
if (this->blacklist) {
auto it = assign_map.find(node->toString());
// Can only inline if driven by identifier, index, or slice
bool valid_driver = it != assign_map.end() &&
(dynamic_cast<Identifier*>(it->second.get()) ||
dynamic_cast<Index*>(it->second.get()) ||
dynamic_cast<Slice*>(it->second.get()));
if (!valid_driver) {
this->wire_blacklist.insert(node->value);
}
blacklist_invalid_driver(node->clone());
}
return node;
}
Expand Down
51 changes: 42 additions & 9 deletions tests/assign_inliner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -802,22 +802,40 @@ TEST(InlineAssignTests, TestNoInlineSliceUnlessID) {
std::make_unique<vAST::Vector>(vAST::make_id("o1"), vAST::make_num("3"),
vAST::make_num("0")),
vAST::OUTPUT, vAST::WIRE));
ports.push_back(std::make_unique<vAST::Port>(
std::make_unique<vAST::Vector>(vAST::make_id("o2"), vAST::make_num("3"),
vAST::make_num("0")),
vAST::OUTPUT, vAST::WIRE));

std::vector<std::variant<std::unique_ptr<vAST::StructuralStatement>,
std::unique_ptr<vAST::Declaration>>>
body;

body.push_back(
std::make_unique<vAST::Wire>(std::make_unique<vAST::Identifier>("x")));
body.push_back(std::make_unique<vAST::Wire>(std::make_unique<vAST::Vector>(
vAST::make_id("x"), vAST::make_num("4"), vAST::make_num("0"))));

body.push_back(
std::make_unique<vAST::Wire>(std::make_unique<vAST::Identifier>("y")));
body.push_back(std::make_unique<vAST::Wire>(std::make_unique<vAST::Vector>(
vAST::make_id("y"), vAST::make_num("4"), vAST::make_num("0"))));

body.push_back(std::make_unique<vAST::Wire>(std::make_unique<vAST::Vector>(
vAST::make_id("h"), vAST::make_num("4"), vAST::make_num("0"))));

body.push_back(std::make_unique<vAST::Wire>(std::make_unique<vAST::Vector>(
vAST::make_id("g"), vAST::make_num("4"), vAST::make_num("0"))));

body.push_back(std::make_unique<vAST::ContinuousAssign>(
vAST::make_id("x"),
std::make_unique<vAST::BinaryOp>(vAST::make_id("i1"), vAST::BinOp::ADD,
vAST::make_id("i2"))));

body.push_back(std::make_unique<vAST::ContinuousAssign>(
vAST::make_id("h"),
std::make_unique<vAST::BinaryOp>(vAST::make_id("i1"), vAST::BinOp::SUB,
vAST::make_id("i2"))));

body.push_back(std::make_unique<vAST::ContinuousAssign>(vAST::make_id("g"),
vAST::make_id("h")));

body.push_back(std::make_unique<vAST::ContinuousAssign>(
vAST::make_id("o0"),
std::make_unique<vAST::Slice>(vAST::make_id("x"), vAST::make_num("3"),
Expand All @@ -831,6 +849,11 @@ TEST(InlineAssignTests, TestNoInlineSliceUnlessID) {
std::make_unique<vAST::Slice>(vAST::make_id("y"), vAST::make_num("3"),
vAST::make_num("0"))));

body.push_back(std::make_unique<vAST::ContinuousAssign>(
vAST::make_id("o2"),
std::make_unique<vAST::Slice>(vAST::make_id("g"), vAST::make_num("3"),
vAST::make_num("0"))));

std::unique_ptr<vAST::AbstractModule> module = std::make_unique<vAST::Module>(
"test_module", std::move(ports), std::move(body));

Expand All @@ -839,14 +862,20 @@ TEST(InlineAssignTests, TestNoInlineSliceUnlessID) {
" input [4:0] i1,\n"
" input [4:0] i2,\n"
" output [3:0] o0,\n"
" output [3:0] o1\n"
" output [3:0] o1,\n"
" output [3:0] o2\n"
");\n"
"wire x;\n"
"wire y;\n"
"wire [4:0] x;\n"
"wire [4:0] y;\n"
"wire [4:0] h;\n"
"wire [4:0] g;\n"
"assign x = i1 + i2;\n"
"assign h = i1 - i2;\n"
"assign g = h;\n"
"assign o0 = x[3:0];\n"
"assign y = i1;\n"
"assign o1 = y[3:0];\n"
"assign o2 = g[3:0];\n"
"endmodule\n";

EXPECT_EQ(module->toString(), raw_str);
Expand All @@ -856,12 +885,16 @@ TEST(InlineAssignTests, TestNoInlineSliceUnlessID) {
" input [4:0] i1,\n"
" input [4:0] i2,\n"
" output [3:0] o0,\n"
" output [3:0] o1\n"
" output [3:0] o1,\n"
" output [3:0] o2\n"
");\n"
"wire x;\n"
"wire [4:0] x;\n"
"wire [4:0] h;\n"
"assign x = i1 + i2;\n"
"assign h = i1 - i2;\n"
"assign o0 = x[3:0];\n"
"assign o1 = i1[3:0];\n"
"assign o2 = h[3:0];\n"
"endmodule\n";

vAST::AssignInliner transformer;
Expand Down

0 comments on commit 0aa2569

Please sign in to comment.