From e4c98e1e4c40c083fcab070f3682dd4f0f779ec0 Mon Sep 17 00:00:00 2001 From: Eric Astor Date: Fri, 21 Aug 2026 06:56:47 -0700 Subject: [PATCH] [codegen 1.5] Teach MergePortsPass how to keep the signature up-to-date This makes it safe to run after signature generation, which is useful in some circumstances. PiperOrigin-RevId: 968476021 --- xls/codegen_v_1_5/BUILD | 5 + xls/codegen_v_1_5/merge_ports_pass.cc | 62 +++++ xls/codegen_v_1_5/merge_ports_pass_test.cc | 255 +++++++++++++++++++++ 3 files changed, 322 insertions(+) diff --git a/xls/codegen_v_1_5/BUILD b/xls/codegen_v_1_5/BUILD index a99e96776a..4e3de04dda 100644 --- a/xls/codegen_v_1_5/BUILD +++ b/xls/codegen_v_1_5/BUILD @@ -222,12 +222,14 @@ xls_codegen_pass( pass_class = "MergePortsPass", deps = [ ":block_conversion_pass", + "//xls/codegen:module_signature_cc_proto", "//xls/common/status:status_macros", "//xls/ir", "//xls/ir:channel", "//xls/passes:pass_base", "@abseil-cpp//absl/container:flat_hash_map", "@abseil-cpp//absl/status:statusor", + "@protobuf//:protobuf_lite", ], ) @@ -238,6 +240,8 @@ cc_test( ":block_conversion_pass", ":merge_ports_pass", "//xls/codegen:codegen_options", + "//xls/codegen:module_signature_cc_proto", + "//xls/common:proto_test_utils", "//xls/common:xls_gunit_main", "//xls/common/status:matchers", "//xls/ir", @@ -248,6 +252,7 @@ cc_test( "//xls/passes:pass_base", "@abseil-cpp//absl/status:statusor", "@googletest//:gtest", + "@protobuf", ], ) diff --git a/xls/codegen_v_1_5/merge_ports_pass.cc b/xls/codegen_v_1_5/merge_ports_pass.cc index a3cfd2188d..60451831d6 100644 --- a/xls/codegen_v_1_5/merge_ports_pass.cc +++ b/xls/codegen_v_1_5/merge_ports_pass.cc @@ -15,6 +15,7 @@ #include "xls/codegen_v_1_5/merge_ports_pass.h" #include +#include #include #include #include @@ -22,6 +23,9 @@ #include "absl/container/flat_hash_map.h" #include "absl/status/statusor.h" +#include "google/protobuf/message_static_reflection.h" +#include "xls/codegen/module_signature.pb.h" +#include "xls/codegen/module_signature.proto.static_reflection.h" #include "xls/codegen_v_1_5/block_conversion_pass.h" #include "xls/common/status/status_macros.h" #include "xls/ir/block.h" @@ -35,6 +39,17 @@ namespace xls::codegen { namespace { +template +void MaybeRemap(Msg& msg, + const absl::flat_hash_map& remap) { + auto field_info = google::protobuf::FieldInfo(); + if (field_info.Has(msg)) { + if (auto it = remap.find(field_info.Get(msg)); it != remap.end()) { + field_info.Set(msg, it->second); + } + } +} + absl::StatusOr MergeOutputPorts(Block* block) { // Collect the channel ports, mapping them back to the associated channels. enum class PortRole { kData, kValid, kReady }; @@ -105,6 +120,18 @@ absl::StatusOr MergeOutputPorts(Block* block) { } } + // If the signature exists, collect the authoritative ports for each port + // we're removing; we'll use them to update the signature in a single pass at + // the end. + absl::flat_hash_map remapped_port; + if (block->GetSignature().has_value()) { + for (const auto& [source, ports] : ports_by_source) { + for (OutputPort* port : ports.equivalent_ports) { + remapped_port[port->name()] = ports.authoritative_port->name(); + } + } + } + for (const auto& [source, ports] : ports_by_source) { if (ports.equivalent_ports.empty()) { // No ports to merge. @@ -137,6 +164,41 @@ absl::StatusOr MergeOutputPorts(Block* block) { } changed = true; } + + if (changed && block->GetSignature().has_value()) { + // We need to update the signature, updating all port references & deleting + // the old ports. + verilog::ModuleSignatureProto signature = *block->GetSignature(); + + for (verilog::ChannelInterfaceProto& channel_interface : + *signature.mutable_channel_interfaces()) { + if (channel_interface.has_streaming()) { + verilog::StreamingChannelInterfaceProto* streaming = + channel_interface.mutable_streaming(); + MaybeRemap<"data_port_name">(*streaming, remapped_port); + MaybeRemap<"ready_port_name">(*streaming, remapped_port); + MaybeRemap<"valid_port_name">(*streaming, remapped_port); + } + if (channel_interface.has_single_value()) { + verilog::SingleValueChannelInterfaceProto* single_value = + channel_interface.mutable_single_value(); + MaybeRemap<"data_port_name">(*single_value, remapped_port); + } + } + + // Now that all the references are updated, we can delete the old ports. + for (auto it = signature.mutable_data_ports()->begin(); + it != signature.mutable_data_ports()->end();) { + if (remapped_port.contains(it->name())) { + it = signature.mutable_data_ports()->erase(it); + } else { + ++it; + } + } + + block->SetSignature(std::move(signature)); + } + return changed; } diff --git a/xls/codegen_v_1_5/merge_ports_pass_test.cc b/xls/codegen_v_1_5/merge_ports_pass_test.cc index 25cf3e8afb..893a75027a 100644 --- a/xls/codegen_v_1_5/merge_ports_pass_test.cc +++ b/xls/codegen_v_1_5/merge_ports_pass_test.cc @@ -20,8 +20,11 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" #include "absl/status/statusor.h" +#include "google/protobuf/text_format.h" #include "xls/codegen/codegen_options.h" +#include "xls/codegen/module_signature.pb.h" #include "xls/codegen_v_1_5/block_conversion_pass.h" +#include "xls/common/proto_test_utils.h" #include "xls/common/status/matchers.h" #include "xls/ir/bits.h" #include "xls/ir/block.h" @@ -37,6 +40,7 @@ namespace { using ::absl_testing::IsOkAndHolds; using ::testing::Optional; +using ::xls::proto_testing::EqualsProto; class MergePortsPassTest : public IrTestBase { protected: @@ -734,5 +738,256 @@ TEST_F(MergePortsPassTest, PartiallySharedPortsMerge) { Optional(std::string{block->GetOutputPorts().front()->name()})); } +TEST_F(MergePortsPassTest, UpdateSignatureStreamingSendDataPorts) { + auto p = CreatePackage(); + BlockBuilder bb(TestName(), p.get()); + BValue in = bb.InputPort("in", p->GetBitsType(32)); + bb.OutputPort("out_a_data", in); + bb.OutputPort("out_b_data", in); + XLS_ASSERT_OK_AND_ASSIGN(Block * block, bb.Build()); + + XLS_ASSERT_OK(block->AddChannelPortMetadata(ChannelPortMetadata{ + .channel_name = "out_a", + .type = p->GetBitsType(32), + .direction = ChannelDirection::kSend, + .channel_kind = ChannelKind::kStreaming, + .flop_kind = FlopKind::kNone, + .data_port = "out_a_data", + })); + XLS_ASSERT_OK(block->AddChannelPortMetadata(ChannelPortMetadata{ + .channel_name = "out_b", + .type = p->GetBitsType(32), + .direction = ChannelDirection::kSend, + .channel_kind = ChannelKind::kStreaming, + .flop_kind = FlopKind::kNone, + .data_port = "out_b_data", + })); + + verilog::ModuleSignatureProto initial_signature; + ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString( + R"pb( + module_name: "test_module" + data_ports { + direction: PORT_DIRECTION_OUTPUT + name: "out_a_data" + width: 32 + type { type_enum: BITS bit_count: 32 } + } + data_ports { + direction: PORT_DIRECTION_OUTPUT + name: "out_b_data" + width: 32 + type { type_enum: BITS bit_count: 32 } + } + channel_interfaces { + channel_name: "out_a" + direction: CHANNEL_DIRECTION_SEND + kind: CHANNEL_KIND_STREAMING + streaming { data_port_name: "out_a_data" } + } + channel_interfaces { + channel_name: "out_b" + direction: CHANNEL_DIRECTION_SEND + kind: CHANNEL_KIND_STREAMING + streaming { data_port_name: "out_b_data" } + } + )pb", + &initial_signature)); + block->SetSignature(initial_signature); + + EXPECT_THAT(Run(p.get()), IsOkAndHolds(true)); + + EXPECT_THAT(block->GetSignature(), Optional(EqualsProto(R"pb( + module_name: "test_module" + data_ports { + direction: PORT_DIRECTION_OUTPUT + name: "out_a_data" + width: 32 + type { type_enum: BITS bit_count: 32 } + } + channel_interfaces { + channel_name: "out_a" + direction: CHANNEL_DIRECTION_SEND + kind: CHANNEL_KIND_STREAMING + streaming { data_port_name: "out_a_data" } + } + channel_interfaces { + channel_name: "out_b" + direction: CHANNEL_DIRECTION_SEND + kind: CHANNEL_KIND_STREAMING + streaming { data_port_name: "out_a_data" } + } + )pb"))); +} + +TEST_F(MergePortsPassTest, UpdateSignatureReceiveReadyPorts) { + auto p = CreatePackage(); + BlockBuilder bb(TestName(), p.get()); + bb.InputPort("in_a_data", p->GetBitsType(32)); + bb.InputPort("in_b_data", p->GetBitsType(32)); + BValue rdy = bb.Literal(UBits(1, 1)); + bb.OutputPort("in_a_rdy", rdy); + bb.OutputPort("in_b_rdy", rdy); + XLS_ASSERT_OK_AND_ASSIGN(Block * block, bb.Build()); + + XLS_ASSERT_OK(block->AddChannelPortMetadata(ChannelPortMetadata{ + .channel_name = "in_a", + .type = p->GetBitsType(32), + .direction = ChannelDirection::kReceive, + .channel_kind = ChannelKind::kStreaming, + .flop_kind = FlopKind::kNone, + .data_port = "in_a_data", + .ready_port = "in_a_rdy", + })); + XLS_ASSERT_OK(block->AddChannelPortMetadata(ChannelPortMetadata{ + .channel_name = "in_b", + .type = p->GetBitsType(32), + .direction = ChannelDirection::kReceive, + .channel_kind = ChannelKind::kStreaming, + .flop_kind = FlopKind::kNone, + .data_port = "in_b_data", + .ready_port = "in_b_rdy", + })); + + verilog::ModuleSignatureProto initial_signature; + ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString( + R"pb( + module_name: "test_module" + data_ports { + direction: PORT_DIRECTION_OUTPUT + name: "in_a_rdy" + width: 1 + type { type_enum: BITS bit_count: 1 } + } + data_ports { + direction: PORT_DIRECTION_OUTPUT + name: "in_b_rdy" + width: 1 + type { type_enum: BITS bit_count: 1 } + } + channel_interfaces { + channel_name: "in_a" + direction: CHANNEL_DIRECTION_RECEIVE + kind: CHANNEL_KIND_STREAMING + streaming { data_port_name: "in_a_data" ready_port_name: "in_a_rdy" } + } + channel_interfaces { + channel_name: "in_b" + direction: CHANNEL_DIRECTION_RECEIVE + kind: CHANNEL_KIND_STREAMING + streaming { data_port_name: "in_b_data" ready_port_name: "in_b_rdy" } + } + )pb", + &initial_signature)); + block->SetSignature(initial_signature); + + EXPECT_THAT(Run(p.get()), IsOkAndHolds(true)); + + EXPECT_THAT( + block->GetSignature(), Optional(EqualsProto(R"pb( + module_name: "test_module" + data_ports { + direction: PORT_DIRECTION_OUTPUT + name: "in_a_rdy" + width: 1 + type { type_enum: BITS bit_count: 1 } + } + channel_interfaces { + channel_name: "in_a" + direction: CHANNEL_DIRECTION_RECEIVE + kind: CHANNEL_KIND_STREAMING + streaming { data_port_name: "in_a_data" ready_port_name: "in_a_rdy" } + } + channel_interfaces { + channel_name: "in_b" + direction: CHANNEL_DIRECTION_RECEIVE + kind: CHANNEL_KIND_STREAMING + streaming { data_port_name: "in_b_data" ready_port_name: "in_a_rdy" } + } + )pb"))); +} + +TEST_F(MergePortsPassTest, UpdateSignatureSingleValueChannels) { + auto p = CreatePackage(); + BlockBuilder bb(TestName(), p.get()); + BValue in = bb.InputPort("in", p->GetBitsType(32)); + bb.OutputPort("sv_a", in); + bb.OutputPort("sv_b", in); + XLS_ASSERT_OK_AND_ASSIGN(Block * block, bb.Build()); + + XLS_ASSERT_OK(block->AddChannelPortMetadata(ChannelPortMetadata{ + .channel_name = "sv_a", + .type = p->GetBitsType(32), + .direction = ChannelDirection::kSend, + .channel_kind = ChannelKind::kSingleValue, + .flop_kind = FlopKind::kNone, + .data_port = "sv_a", + })); + XLS_ASSERT_OK(block->AddChannelPortMetadata(ChannelPortMetadata{ + .channel_name = "sv_b", + .type = p->GetBitsType(32), + .direction = ChannelDirection::kSend, + .channel_kind = ChannelKind::kSingleValue, + .flop_kind = FlopKind::kNone, + .data_port = "sv_b", + })); + + verilog::ModuleSignatureProto initial_signature; + ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString( + R"pb( + module_name: "test_module" + data_ports { + direction: PORT_DIRECTION_OUTPUT + name: "sv_a" + width: 32 + type { type_enum: BITS bit_count: 32 } + } + data_ports { + direction: PORT_DIRECTION_OUTPUT + name: "sv_b" + width: 32 + type { type_enum: BITS bit_count: 32 } + } + channel_interfaces { + channel_name: "sv_a" + direction: CHANNEL_DIRECTION_SEND + kind: CHANNEL_KIND_SINGLE_VALUE + single_value { data_port_name: "sv_a" } + } + channel_interfaces { + channel_name: "sv_b" + direction: CHANNEL_DIRECTION_SEND + kind: CHANNEL_KIND_SINGLE_VALUE + single_value { data_port_name: "sv_b" } + } + )pb", + &initial_signature)); + block->SetSignature(initial_signature); + + EXPECT_THAT(Run(p.get()), IsOkAndHolds(true)); + + EXPECT_THAT(block->GetSignature(), Optional(EqualsProto(R"pb( + module_name: "test_module" + data_ports { + direction: PORT_DIRECTION_OUTPUT + name: "sv_a" + width: 32 + type { type_enum: BITS bit_count: 32 } + } + channel_interfaces { + channel_name: "sv_a" + direction: CHANNEL_DIRECTION_SEND + kind: CHANNEL_KIND_SINGLE_VALUE + single_value { data_port_name: "sv_a" } + } + channel_interfaces { + channel_name: "sv_b" + direction: CHANNEL_DIRECTION_SEND + kind: CHANNEL_KIND_SINGLE_VALUE + single_value { data_port_name: "sv_a" } + } + )pb"))); +} + } // namespace } // namespace xls::codegen