Skip to content

Commit 9f87d49

Browse files
povikpovik
authored
Merge pull request #10755 from The-OpenROAD-Project-staging/syn-fixes
Fix two syn correctness bugs
2 parents daa0fbe + 6f714d0 commit 9f87d49

10 files changed

Lines changed: 260 additions & 35 deletions

File tree

‎src/syn/BUILD‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ cc_library(
2323
"src/flow/export.cc",
2424
"src/flow/export.h",
2525
"src/flow/import.cc",
26-
"src/flow/import.h",
2726
"src/flow/liveness.cc",
2827
"src/flow/opt_gatefusion.cc",
2928
"src/flow/opt_gatefusion.h",
@@ -33,6 +32,7 @@ cc_library(
3332
hdrs = [
3433
"include/syn/synthesis.h",
3534
"src/flow/constant_fold.h",
35+
"src/flow/import.h",
3636
],
3737
includes = [
3838
"include",

‎src/syn/src/elab/driver.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ std::unique_ptr<Graph> elaborate(utl::Logger* logger,
3232
const std::vector<std::string>& args,
3333
sta::dbSta* sta = nullptr);
3434
std::unique_ptr<Graph> elaborateText(const std::string& source,
35-
const std::vector<std::string>& args = {});
35+
const std::vector<std::string>& args = {},
36+
sta::dbSta* sta = nullptr);
3637

3738
} // namespace syn

‎src/syn/src/elab/error.cc‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,11 @@ std::unique_ptr<Graph> elaborateImpl(utl::Logger* logger,
2929
std::optional<std::string> buffer);
3030

3131
std::unique_ptr<Graph> elaborateText(const std::string& source,
32-
const std::vector<std::string>& args)
32+
const std::vector<std::string>& args,
33+
sta::dbSta* sta)
3334
{
3435
utl::Logger logger;
35-
return elaborateImpl(&logger, args, nullptr, source);
36+
return elaborateImpl(&logger, args, sta, source);
3637
}
3738

3839
} // namespace syn

‎src/syn/src/flow/combinational_mapper.cc‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -490,15 +490,16 @@ ClassMatch* Mapping::allocMatches(const int n)
490490

491491
void Mapping::collectPrimaryOutputs()
492492
{
493-
std::set<ControlNet> primaryOutputNets;
494-
std::set<std::pair<ControlNet, Net>> fixupSet;
493+
std::set<ControlNet> primary_output_nets;
494+
std::set<std::pair<ControlNet, Net>> fixup_set;
495495

496496
auto registerPrimaryOutput = [&](Net net) {
497497
auto cnet = subject_.stripInverter(net);
498-
if (isMappable(cnet.net())) {
499-
primaryOutputNets.insert(cnet);
498+
auto driver = subject_.graph.resolve(cnet.net()).first;
499+
if (isMappable(cnet.net()) || driver->isMapped()) {
500+
primary_output_nets.insert(cnet);
500501
if (cnet.net() != net) {
501-
fixupSet.insert({cnet, net});
502+
fixup_set.insert({cnet, net});
502503
}
503504
}
504505
};
@@ -529,8 +530,9 @@ void Mapping::collectPrimaryOutputs()
529530
}
530531
});
531532

532-
primary_outputs_.assign(primaryOutputNets.begin(), primaryOutputNets.end());
533-
primary_output_fixups_.assign(fixupSet.begin(), fixupSet.end());
533+
primary_outputs_.assign(primary_output_nets.begin(),
534+
primary_output_nets.end());
535+
primary_output_fixups_.assign(fixup_set.begin(), fixup_set.end());
534536
}
535537

536538
void Mapping::computeFanouts()

‎src/syn/src/flow/import.cc‎

Lines changed: 78 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55

66
#include <cstdint>
77
#include <cstdlib>
8+
#include <map>
9+
#include <string_view>
810
#include <utility>
911

1012
#include "sta/Liberty.hh"
@@ -31,7 +33,8 @@ void importTargets(Graph& g, sta::Network* network, utl::Logger* logger)
3133
return;
3234
}
3335

34-
// Build expected input/output widths from liberty cell ports.
36+
std::map<std::string_view, uint32_t> lib_input_offset;
37+
std::map<std::string_view, uint32_t> lib_output_offset;
3538
uint32_t lib_input_width = 0;
3639
uint32_t lib_output_width = 0;
3740
sta::LibertyCellPortIterator port_iter(cell);
@@ -42,10 +45,14 @@ void importTargets(Graph& g, sta::Network* network, utl::Logger* logger)
4245
}
4346
sta::PortDirection* dir = port->direction();
4447
if (dir->isInput()) {
48+
lib_input_offset[port->name()] = lib_input_width;
4549
lib_input_width += port->size();
4650
} else if (dir->isOutput()) {
51+
lib_output_offset[port->name()] = lib_output_width;
4752
lib_output_width += port->size();
4853
} else if (dir->isBidirect()) {
54+
lib_input_offset[port->name()] = lib_input_width;
55+
lib_output_offset[port->name()] = lib_output_width;
4956
lib_input_width += port->size();
5057
lib_output_width += port->size();
5158
}
@@ -66,8 +73,7 @@ void importTargets(Graph& g, sta::Network* network, utl::Logger* logger)
6673
logger->error(utl::SYN,
6774
10,
6875
"importTargets: cell '{}' input width mismatch:"
69-
" Other has {} bits, Liberty has {} bits. This is an "
70-
"internal tool error.",
76+
" Verilog instantiation has {} bits, Liberty has {} bits.",
7177
other->cellType(),
7278
other_input_width,
7379
lib_input_width);
@@ -77,26 +83,89 @@ void importTargets(Graph& g, sta::Network* network, utl::Logger* logger)
7783
logger->error(utl::SYN,
7884
11,
7985
"importTargets: cell '{}' output width mismatch:"
80-
" Other has {} bits, Liberty has {} bits. This is an "
81-
"internal tool error.",
86+
" Verilog instantiation has {} bits, Liberty has {} bits.",
8287
other->cellType(),
8388
other_output_width,
8489
lib_output_width);
8590
}
8691

8792
// Concatenate all input port values into a single Bundle.
88-
Bundle inputs;
93+
Bundle inputs = Bundle::sentinel(lib_input_width);
8994
for (auto& port : other->ports()) {
9095
if (port.direction == Other::Port::kInput
9196
|| port.direction == Other::Port::kInOut) {
92-
inputs = inputs.concat(port.value);
97+
sta::LibertyPort* lib_port = cell->findLibertyPort(port.name);
98+
if (!lib_port || !lib_input_offset.contains(port.name)) {
99+
logger->error(
100+
utl::SYN,
101+
35,
102+
"importTargets: cell '{}' Liberty definition is missing "
103+
"port '{}' connected in Verilog instantiation.",
104+
other->cellType(),
105+
port.name);
106+
}
107+
108+
if (((uint32_t) lib_port->size()) != port.width) {
109+
logger->error(
110+
utl::SYN,
111+
36,
112+
"importTargets: cell '{}' instantiated with mismatched width "
113+
"of port '{}': {} in Verilog vs {} in Liberty",
114+
other->cellType(),
115+
port.name,
116+
port.width,
117+
lib_port->size());
118+
}
119+
120+
uint32_t offset = lib_input_offset.at(port.name);
121+
for (uint32_t i = 0; i < port.width; i++) {
122+
inputs.mutableNet(offset + i) = port.value[i];
123+
}
93124
}
94125
}
95126

96127
// Create Target and replace outputs.
97128
BundleView old_output = g.output(other);
98-
Bundle new_output = g.add<Target>(cell, std::move(inputs));
99-
g.forceReplace(old_output, BundleView(new_output));
129+
Bundle target_output = g.add<Target>(cell, std::move(inputs));
130+
Bundle output_replacement = Bundle::sentinel(lib_output_width);
131+
132+
uint32_t offset = 0;
133+
for (auto& port : other->ports()) {
134+
if (port.direction == Other::Port::kOutput
135+
|| port.direction == Other::Port::kInOut) {
136+
sta::LibertyPort* lib_port = cell->findLibertyPort(port.name);
137+
if (!lib_port || !lib_output_offset.contains(port.name)) {
138+
logger->error(
139+
utl::SYN,
140+
37,
141+
"importTargets: cell '{}' Liberty definition is missing "
142+
"port '{}' connected in Verilog instantiation.",
143+
other->cellType(),
144+
port.name);
145+
}
146+
147+
if (((uint32_t) lib_port->size()) != port.width) {
148+
logger->error(
149+
utl::SYN,
150+
38,
151+
"importTargets: cell '{}' instantiated with mismatched width "
152+
"of port '{}': {} in Verilog vs {} in Liberty",
153+
other->cellType(),
154+
port.name,
155+
port.width,
156+
lib_port->size());
157+
}
158+
159+
uint32_t lib_offset = lib_output_offset.at(port.name);
160+
for (uint32_t i = 0; i < port.width; i++) {
161+
output_replacement.mutableNet(offset + i)
162+
= target_output[lib_offset + i];
163+
}
164+
offset += port.width;
165+
}
166+
}
167+
168+
g.forceReplace(old_output, BundleView(output_replacement));
100169
g.removeInstance(
101170
const_cast<Instance*>(static_cast<const Instance*>(other)));
102171
});

‎src/syn/test/BUILD‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,9 +115,12 @@ cc_test(
115115
"elab_testcases.h",
116116
"equivalence_check.h",
117117
],
118+
data = ["macro_test_cells.lib"],
118119
deps = [
120+
"//src/syn",
119121
"//src/syn/src/elab",
120122
"//src/syn/src/ir",
123+
"//src/tst",
121124
"@googletest//:gtest",
122125
],
123126
)

‎src/syn/test/CMakeLists.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,6 @@ foreach(_t abc_test sm_test cm_test)
4040
endforeach()
4141

4242
add_executable(elab_test elab_test.cc)
43-
target_link_libraries(elab_test ${_syn_test_libs} syn_elab syn_ir)
43+
target_link_libraries(elab_test ${_syn_test_libs} syn_elab syn_flow syn_ir tst)
4444
gtest_discover_tests(elab_test WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})
4545
add_dependencies(build_and_test elab_test)

‎src/syn/test/cm_test.cc‎

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -209,21 +209,22 @@ TEST_P(CmTest, MapsBelowAreaAndIsEquivalent)
209209
// naming-only Nots that cm.cc:integrate() leaves behind.
210210
float area = 0.0f;
211211
gate->forEachInstance([&](const Instance* inst) {
212-
if (inst->is<Input>() || inst->is<Output>() || inst->is<Name>()
213-
|| inst->is<Not>() || inst->is<TieLow>() || inst->is<TieHigh>()
214-
|| inst->is<TieX>()) {
215-
return;
212+
if (inst->isMapped()) {
213+
inst->visit([&](Net fanin) {
214+
auto [fanin_driver, offset] = gate->resolve(fanin);
215+
if (!fanin_driver->isMapped()) {
216+
std::ostringstream os;
217+
gate->dumpInstance(os, fanin_driver);
218+
ADD_FAILURE() << "Unexpected instance after combinational mapping: "
219+
<< os.str();
220+
return;
221+
}
222+
});
216223
}
217-
if (!inst->is<Target>()) {
218-
std::ostringstream os;
219-
gate->dumpInstance(os, inst);
220-
ADD_FAILURE() << "Non-Target instance survived combinational mapping in "
221-
"case "
222-
<< tc.name << ": " << os.str();
223-
return;
224-
}
225-
if (auto* t = inst->try_as<Target>()) {
226-
area += t->cell()->area();
224+
if (inst->is<Target>()) {
225+
if (auto* t = inst->try_as<Target>()) {
226+
area += t->cell()->area();
227+
}
227228
}
228229
});
229230

‎src/syn/test/elab_test.cc‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,14 @@
1111
#include "driver.h"
1212
#include "elab_testcases.h"
1313
#include "equivalence_check.h"
14+
#include "flow/import.h"
1415
#include "gtest/gtest.h"
1516
#include "syn/ir/Bundle.h"
1617
#include "syn/ir/Graph.h"
1718
#include "syn/ir/Instance.h"
1819
#include "syn/ir/TritModel.h"
20+
#include "syn/synthesis.h"
21+
#include "tst/fixture.h"
1922

2023
int main(int argc, char** argv)
2124
{
@@ -289,4 +292,82 @@ INSTANTIATE_TEST_SUITE_P(Elab,
289292
ElabSlangTest,
290293
::testing::ValuesIn(elabTestcases));
291294

295+
class ElabWithMacrosTest : public tst::Fixture
296+
{
297+
protected:
298+
void SetUp() override
299+
{
300+
Fixture::SetUp();
301+
readLiberty(getFilePath("_main/src/syn/test/macro_test_cells.lib"));
302+
}
303+
};
304+
305+
TEST_F(ElabWithMacrosTest, BlackboxImport)
306+
{
307+
// Check macro connections are correct even when Verilog blackbox
308+
// module does not match Liberty port order
309+
auto result = syn::elaborateText(R"(
310+
module top(
311+
input logic clk,
312+
input logic ce,
313+
input logic we,
314+
input logic [8:0] addr,
315+
input logic [7:0] wd,
316+
output logic [7:0] rd
317+
);
318+
fakeram_512x8 u_ram (
319+
.rd_out(rd),
320+
.addr_in(addr),
321+
.we_in(we),
322+
.wd_in(wd),
323+
.clk(clk),
324+
.ce_in(ce)
325+
);
326+
endmodule
327+
328+
(* blackbox *)
329+
module fakeram_512x8 (
330+
output logic [7:0] rd_out,
331+
input logic [8:0] addr_in,
332+
input logic we_in,
333+
input logic [7:0] wd_in,
334+
input logic clk,
335+
input logic ce_in
336+
);
337+
endmodule
338+
)",
339+
{"--top", "top"},
340+
getSta());
341+
ASSERT_TRUE(result) << "Elaboration failed";
342+
343+
syn::Graph& g = *result;
344+
g.normalize();
345+
importTargets(g, getSta()->network(), getLogger());
346+
std::ostringstream os;
347+
g.dump(os);
348+
EXPECT_EQ(os.str(),
349+
R"(%3:0 = name "clk" 0 1 %9
350+
%4:0 = name "ce" 0 1 %10
351+
%5:0 = name "we" 0 1 %11
352+
%6:0 = name "addr" 0 9 vector %12:9
353+
%7:0 = name "wd" 0 8 vector %21:8
354+
%8:0 = name "rd" 0 8 vector %30:8
355+
%9:1 = input "clk"
356+
%10:1 = input "ce"
357+
%11:1 = input "we"
358+
%12:9 = input "addr"
359+
%21:8 = input "wd"
360+
%29:0 = output "rd" %30:8
361+
%30:8 = buf %38:8
362+
%38:8 = target "fakeram_512x8" {
363+
input "clk" = %9
364+
%38:8 = output "rd_out"
365+
input "we_in" = %11
366+
input "ce_in" = %10
367+
input "addr_in" = %12:9
368+
input "wd_in" = %21:8
369+
}
370+
)");
371+
}
372+
292373
} // namespace syn

0 commit comments

Comments
 (0)