Skip to content

Commit 07f4cde

Browse files
meheffernancopybara-github
meheffernan
authored and
copybara-github
committed
Move ModuleSignature from bag-on-the-side metadata to xls::Block.
The signature is stored as a ModuleSignatureProto on the block. This requires converting it to a ModuleSignature then writing it back to mutate it, but the module signature is small and mutated infrequently. PiperOrigin-RevId: 760688271
1 parent 1f399e4 commit 07f4cde

18 files changed

+164
-107
lines changed

‎xls/codegen/BUILD

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -622,6 +622,7 @@ cc_library(
622622
hdrs = ["signature_generation_pass.h"],
623623
deps = [
624624
":codegen_pass",
625+
":module_signature",
625626
":signature_generator",
626627
"//xls/common/status:status_macros",
627628
"//xls/ir",
@@ -1440,6 +1441,7 @@ cc_library(
14401441
deps = [
14411442
":block_metrics",
14421443
":codegen_pass",
1444+
":module_signature",
14431445
":xls_metrics_cc_proto",
14441446
"//xls/common/status:status_macros",
14451447
"//xls/ir",
@@ -1709,6 +1711,7 @@ cc_test(
17091711
":codegen_options",
17101712
":codegen_pass",
17111713
":codegen_pass_pipeline",
1714+
":module_signature",
17121715
":module_signature_cc_proto",
17131716
":ram_configuration",
17141717
":ram_rewrite_pass",

‎xls/codegen/block_metrics_generation_pass.cc

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include "absl/status/statusor.h"
1919
#include "xls/codegen/block_metrics.h"
2020
#include "xls/codegen/codegen_pass.h"
21+
#include "xls/codegen/module_signature.h"
2122
#include "xls/codegen/xls_metrics.pb.h"
2223
#include "xls/common/status/status_macros.h"
2324
#include "xls/ir/package.h"
@@ -30,13 +31,18 @@ absl::StatusOr<bool> BlockMetricsGenerationPass::RunInternal(
3031
CodegenContext& context) const {
3132
bool changed = false;
3233
for (auto& [block, metadata] : context.metadata()) {
33-
if (!metadata.signature.has_value()) {
34+
if (!block->GetSignature().has_value()) {
3435
return absl::InvalidArgumentError(
3536
"Block metrics should be run after signature generation.");
3637
}
3738
XLS_ASSIGN_OR_RETURN(BlockMetricsProto block_metrics,
3839
GenerateBlockMetrics(block, options.delay_estimator));
39-
XLS_RETURN_IF_ERROR(metadata.signature->ReplaceBlockMetrics(block_metrics));
40+
41+
XLS_ASSIGN_OR_RETURN(ModuleSignature signature,
42+
ModuleSignature::FromProto(*block->GetSignature()));
43+
XLS_RETURN_IF_ERROR(signature.ReplaceBlockMetrics(block_metrics));
44+
block->SetSignature(signature.proto());
45+
4046
changed = true;
4147
}
4248

‎xls/codegen/codegen_pass.h

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,8 @@
2828
#include "absl/container/btree_map.h"
2929
#include "absl/container/flat_hash_map.h"
3030
#include "absl/log/check.h"
31-
#include "absl/types/span.h"
3231
#include "xls/codegen/codegen_options.h"
3332
#include "xls/codegen/concurrent_stage_groups.h"
34-
#include "xls/codegen/module_signature.h"
3533
#include "xls/codegen/passes_ng/stage_conversion.h"
3634
#include "xls/estimators/delay_model/delay_estimator.h"
3735
#include "xls/ir/block.h"
@@ -343,14 +341,6 @@ struct CodegenMetadata {
343341
std::variant<FunctionConversionMetadata, ProcConversionMetadata>
344342
conversion_metadata;
345343

346-
// The signature is generated (and potentially mutated) during the codegen
347-
// process.
348-
// TODO(https://github.com/google/xls/issues/410): 2021/04/27 Consider adding
349-
// a "block" construct which corresponds to a verilog module. This block could
350-
// hold its own signature. This would help prevent the signature from getting
351-
// out-of-sync with the IR.
352-
std::optional<ModuleSignature> signature;
353-
354344
// Proven knowledge about which stages are active concurrently.
355345
//
356346
// If absent all stages should be considered potentially concurrently active

‎xls/codegen/combinational_generator.cc

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#include "xls/codegen/combinational_generator.h"
1616

1717
#include <string>
18+
#include <utility>
1819

1920
#include "absl/status/statusor.h"
2021
#include "xls/codegen/block_conversion.h"
@@ -52,8 +53,7 @@ absl::StatusOr<ModuleGeneratorResult> GenerateCombinationalModule(
5253
.status());
5354
XLS_RET_CHECK_NE(context.top_block(), nullptr);
5455
XLS_RET_CHECK(context.metadata().contains(context.top_block()));
55-
XLS_RET_CHECK(
56-
context.metadata().at(context.top_block()).signature.has_value());
56+
XLS_RET_CHECK(context.top_block()->GetSignature().has_value());
5757
VerilogLineMap verilog_line_map;
5858
XLS_ASSIGN_OR_RETURN(
5959
std::string verilog,
@@ -63,11 +63,13 @@ absl::StatusOr<ModuleGeneratorResult> GenerateCombinationalModule(
6363
*metrics = results.ToProto();
6464
}
6565

66+
XLS_ASSIGN_OR_RETURN(
67+
ModuleSignature signature,
68+
ModuleSignature::FromProto(*context.top_block()->GetSignature()));
69+
6670
// TODO: google/xls#1323 - add all block signatures to ModuleGeneratorResult,
6771
// not just top.
68-
return ModuleGeneratorResult{
69-
verilog, verilog_line_map,
70-
context.metadata().at(context.top_block()).signature.value()};
72+
return ModuleGeneratorResult{verilog, verilog_line_map, std::move(signature)};
7173
}
7274

7375
} // namespace verilog

‎xls/codegen/pipeline_generator.cc

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -83,10 +83,10 @@ absl::StatusOr<ModuleGeneratorResult> ToPipelineModuleText(
8383
if (metrics != nullptr) {
8484
*metrics = results.ToProto();
8585
}
86-
XLS_RET_CHECK(
87-
context.top_block() != nullptr &&
88-
context.HasMetadataForBlock(context.top_block()) &&
89-
context.GetMetadataForBlock(context.top_block()).signature.has_value());
86+
XLS_RET_CHECK(context.top_block() != nullptr &&
87+
context.HasMetadataForBlock(context.top_block()) &&
88+
context.top_block()->GetSignature().has_value());
89+
9090
VerilogLineMap verilog_line_map;
9191
const auto& pipeline = context.GetMetadataForBlock(context.top_block())
9292
.streaming_io_and_pipeline;
@@ -96,11 +96,13 @@ absl::StatusOr<ModuleGeneratorResult> ToPipelineModuleText(
9696
&verilog_line_map, pipeline.input_port_sv_type,
9797
pipeline.output_port_sv_type));
9898

99+
XLS_ASSIGN_OR_RETURN(
100+
ModuleSignature signature,
101+
ModuleSignature::FromProto(*context.top_block()->GetSignature()));
102+
99103
// TODO: google/xls#1323 - add all block signatures to ModuleGeneratorResult,
100104
// not just top.
101-
return ModuleGeneratorResult{
102-
verilog, verilog_line_map,
103-
context.GetMetadataForBlock(context.top_block()).signature.value()};
105+
return ModuleGeneratorResult{verilog, verilog_line_map, std::move(signature)};
104106
}
105107

106108
absl::StatusOr<ModuleGeneratorResult> ToPipelineModuleText(
@@ -143,10 +145,9 @@ absl::StatusOr<ModuleGeneratorResult> ToPipelineModuleText(
143145
*metrics = results.ToProto();
144146
}
145147

146-
XLS_RET_CHECK(
147-
context.top_block() != nullptr &&
148-
context.HasMetadataForBlock(context.top_block()) &&
149-
context.GetMetadataForBlock(context.top_block()).signature.has_value());
148+
XLS_RET_CHECK(context.top_block() != nullptr &&
149+
context.HasMetadataForBlock(context.top_block()) &&
150+
context.top_block()->GetSignature().has_value());
150151
VerilogLineMap verilog_line_map;
151152
const auto& pipeline = context.GetMetadataForBlock(context.top_block())
152153
.streaming_io_and_pipeline;
@@ -156,11 +157,13 @@ absl::StatusOr<ModuleGeneratorResult> ToPipelineModuleText(
156157
pipeline.input_port_sv_type,
157158
pipeline.output_port_sv_type));
158159

160+
XLS_ASSIGN_OR_RETURN(
161+
ModuleSignature signature,
162+
ModuleSignature::FromProto(*context.top_block()->GetSignature()));
163+
159164
// TODO: google/xls#1323 - add all block signatures to ModuleGeneratorResult,
160165
// not just top.
161-
return ModuleGeneratorResult{
162-
verilog, verilog_line_map,
163-
context.GetMetadataForBlock(context.top_block()).signature.value()};
166+
return ModuleGeneratorResult{verilog, verilog_line_map, std::move(signature)};
164167
}
165168

166169
} // namespace verilog

‎xls/codegen/ram_rewrite_pass.cc

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -538,8 +538,9 @@ absl::StatusOr<bool> Ram1RWRewrite(Package* package,
538538
ram_config.rw_port_configuration().response_channel_name,
539539
ram_config.rw_port_configuration().write_completion_channel_name});
540540

541-
if (metadata.signature.has_value()) {
542-
ModuleSignature& signature = *metadata.signature;
541+
if (block->GetSignature().has_value()) {
542+
XLS_ASSIGN_OR_RETURN(ModuleSignature signature,
543+
ModuleSignature::FromProto(*block->GetSignature()));
543544
XLS_RETURN_IF_ERROR(
544545
Ram1RWUpdateSignature(signature, ram_config, ram_name, package,
545546
/*req_addr_port=*/req_addr_port,
@@ -549,6 +550,7 @@ absl::StatusOr<bool> Ram1RWRewrite(Package* package,
549550
/*req_wr_mask_port=*/req_wr_mask_port,
550551
/*req_rd_mask_port=*/req_rd_mask_port,
551552
/*resp_rd_data_port=*/resp_rd_data_port));
553+
block->SetSignature(signature.proto());
552554
}
553555
}
554556

@@ -722,7 +724,7 @@ absl::StatusOr<bool> Ram1R1WRewrite(Package* package,
722724
CodegenMetadata& metadata =
723725
context.GetMetadataForBlock(context.top_block());
724726

725-
if (metadata.signature.has_value()) {
727+
if (context.top_block()->GetSignature().has_value()) {
726728
ClearRewrittenMetadata(
727729
metadata.streaming_io_and_pipeline,
728730
{
@@ -731,8 +733,11 @@ absl::StatusOr<bool> Ram1R1WRewrite(Package* package,
731733
ram_config.w_port_configuration().request_channel_name,
732734
ram_config.w_port_configuration().write_completion_channel_name,
733735
});
734-
ModuleSignature& signature = metadata.signature.value();
735-
auto builder = ModuleSignatureBuilder::FromProto(signature.proto());
736+
XLS_ASSIGN_OR_RETURN(
737+
ModuleSignature signature,
738+
ModuleSignature::FromProto(*context.top_block()->GetSignature()));
739+
auto builder = ModuleSignatureBuilder::FromProto(
740+
*context.top_block()->GetSignature());
736741

737742
for (std::string_view channel_name : {
738743
ram_config.r_port_configuration().request_channel_name,
@@ -797,6 +802,7 @@ absl::StatusOr<bool> Ram1R1WRewrite(Package* package,
797802
});
798803

799804
XLS_ASSIGN_OR_RETURN(signature, builder.Build());
805+
context.top_block()->SetSignature(signature.proto());
800806
}
801807
}
802808

‎xls/codegen/ram_rewrite_pass_test.cc

Lines changed: 17 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
#include "xls/codegen/codegen_options.h"
4343
#include "xls/codegen/codegen_pass.h"
4444
#include "xls/codegen/codegen_pass_pipeline.h"
45+
#include "xls/codegen/module_signature.h"
4546
#include "xls/codegen/module_signature.pb.h"
4647
#include "xls/codegen/ram_configuration.h"
4748
#include "xls/common/proto_test_utils.h"
@@ -481,16 +482,17 @@ TEST_P(RamRewritePassTest, ModuleSignatureUpdated) {
481482

482483
EXPECT_TRUE(changed);
483484

484-
ASSERT_TRUE(
485-
context.HasMetadataForBlock(context.top_block()) &&
486-
context.GetMetadataForBlock(context.top_block()).signature.has_value());
485+
ASSERT_TRUE(context.HasMetadataForBlock(context.top_block()) &&
486+
context.top_block()->GetSignature().has_value());
487+
XLS_ASSERT_OK_AND_ASSIGN(
488+
ModuleSignature signature,
489+
ModuleSignature::FromProto(*context.top_block()->GetSignature()));
487490
for (const auto& config : codegen_options.ram_configurations()) {
488491
absl::flat_hash_set<std::string> channel_names;
489492
if (std::holds_alternative<Ram1RWConfiguration>(config)) {
490493
auto ram1rw_config = std::get<Ram1RWConfiguration>(config);
491494
bool found = false;
492-
for (auto& ram :
493-
context.GetMetadataForBlock(context.top_block()).signature->rams()) {
495+
for (auto& ram : signature.rams()) {
494496
if (ram.ram_oneof_case() != RamProto::RamOneofCase::kRam1Rw) {
495497
continue;
496498
}
@@ -502,9 +504,7 @@ TEST_P(RamRewritePassTest, ModuleSignatureUpdated) {
502504
}
503505
// Check the port information matches.
504506
EXPECT_THAT(
505-
context.GetMetadataForBlock(context.top_block())
506-
.signature->proto()
507-
.data_ports(),
507+
signature.proto().data_ports(),
508508
IsSupersetOf({
509509
EqualsProto(ram.ram_1rw().rw_port().response().read_data()),
510510
EqualsProto(ram.ram_1rw().rw_port().request().address()),
@@ -539,8 +539,7 @@ TEST_P(RamRewritePassTest, ModuleSignatureUpdated) {
539539
package->GetTupleType({}))
540540
? 0
541541
: 1;
542-
EXPECT_THAT(context.GetMetadataForBlock(context.top_block())
543-
.signature->data_outputs(),
542+
EXPECT_THAT(signature.data_outputs(),
544543
AllOf(Contains(PortProtoByName(
545544
absl::StrCat(ram1rw_config.ram_name(), "_addr"))),
546545
Contains(PortProtoByName(absl::StrCat(
@@ -555,15 +554,13 @@ TEST_P(RamRewritePassTest, ModuleSignatureUpdated) {
555554
Contains(PortProtoByName(absl::StrCat(
556555
ram1rw_config.ram_name(), "_rd_mask")))
557556
.Times(read_mask_times)));
558-
EXPECT_THAT(context.GetMetadataForBlock(context.top_block())
559-
.signature->data_inputs(),
557+
EXPECT_THAT(signature.data_inputs(),
560558
Contains(PortProtoByName(
561559
absl::StrCat(ram1rw_config.ram_name(), "_rd_data"))));
562560
} else if (std::holds_alternative<Ram1R1WConfiguration>(config)) {
563561
auto ram1r1w_config = std::get<Ram1R1WConfiguration>(config);
564562
bool found = false;
565-
for (auto& ram :
566-
context.GetMetadataForBlock(context.top_block()).signature->rams()) {
563+
for (auto& ram : signature.rams()) {
567564
if (ram.ram_oneof_case() != RamProto::RamOneofCase::kRam1R1W) {
568565
continue;
569566
}
@@ -575,9 +572,7 @@ TEST_P(RamRewritePassTest, ModuleSignatureUpdated) {
575572
}
576573
// Check the port information matches.
577574
EXPECT_THAT(
578-
context.GetMetadataForBlock(context.top_block())
579-
.signature->proto()
580-
.data_ports(),
575+
signature.proto().data_ports(),
581576
IsSupersetOf({
582577
EqualsProto(ram.ram_1r1w().r_port().response().data()),
583578
EqualsProto(ram.ram_1r1w().r_port().request().address()),
@@ -621,8 +616,7 @@ TEST_P(RamRewritePassTest, ModuleSignatureUpdated) {
621616
package->GetTupleType({}))
622617
? 0
623618
: 1;
624-
EXPECT_THAT(context.GetMetadataForBlock(context.top_block())
625-
.signature->data_outputs(),
619+
EXPECT_THAT(signature.data_outputs(),
626620
AllOf(Contains(PortProtoByName(absl::StrCat(
627621
ram1r1w_config.ram_name(), "_rd_addr"))),
628622
Contains(PortProtoByName(absl::StrCat(
@@ -639,24 +633,20 @@ TEST_P(RamRewritePassTest, ModuleSignatureUpdated) {
639633
Contains(PortProtoByName(absl::StrCat(
640634
ram1r1w_config.ram_name(), "_wr_mask")))
641635
.Times(write_mask_times)));
642-
EXPECT_THAT(context.GetMetadataForBlock(context.top_block())
643-
.signature->data_inputs(),
636+
EXPECT_THAT(signature.data_inputs(),
644637
Contains(PortProtoByName(
645638
absl::StrCat(ram1r1w_config.ram_name(), "_rd_data"))));
646639
}
647-
for (auto& channel : context.GetMetadataForBlock(context.top_block())
648-
.signature->GetChannelInterfaces()) {
640+
for (auto& channel : signature.GetChannelInterfaces()) {
649641
EXPECT_THAT(channel_names, Not(Contains(Eq(channel.channel_name()))));
650642
}
651643
for (auto& channel_name : channel_names) {
652-
EXPECT_THAT(context.GetMetadataForBlock(context.top_block())
653-
.signature->data_inputs(),
644+
EXPECT_THAT(signature.data_inputs(),
654645
Not(Contains(AnyOf(
655646
PortProtoByName(channel_name),
656647
PortProtoByName(absl::StrCat(channel_name, "_valid")),
657648
PortProtoByName(absl::StrCat(channel_name, "_ready"))))));
658-
EXPECT_THAT(context.GetMetadataForBlock(context.top_block())
659-
.signature->data_outputs(),
649+
EXPECT_THAT(signature.data_outputs(),
660650
Not(Contains(AnyOf(
661651
PortProtoByName(channel_name),
662652
PortProtoByName(absl::StrCat(channel_name, "_valid")),

‎xls/codegen/signature_generation_pass.cc

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#include "absl/status/statusor.h"
2020
#include "absl/strings/str_format.h"
2121
#include "xls/codegen/codegen_pass.h"
22+
#include "xls/codegen/module_signature.h"
2223
#include "xls/codegen/signature_generator.h"
2324
#include "xls/common/status/status_macros.h"
2425
#include "xls/ir/package.h"
@@ -33,14 +34,15 @@ absl::StatusOr<bool> SignatureGenerationPass::RunInternal(
3334
VLOG(3) << absl::StreamFormat("Metadata has %d blocks",
3435
context.metadata().size());
3536
for (auto& [block, metadata] : context.metadata()) {
36-
if (metadata.signature.has_value()) {
37+
if (block->GetSignature().has_value()) {
3738
return absl::InvalidArgumentError("Signature already generated.");
3839
}
3940
XLS_ASSIGN_OR_RETURN(
40-
metadata.signature,
41+
ModuleSignature signature,
4142
GenerateSignature(
4243
options.codegen_options, block,
4344
metadata.streaming_io_and_pipeline.node_to_stage_map));
45+
block->SetSignature(signature.proto());
4446
changed = true;
4547
}
4648
return changed;

0 commit comments

Comments
 (0)