From f0c8238b452e71045c370c0575c3d6951deb5a94 Mon Sep 17 00:00:00 2001 From: Eric Astor Date: Mon, 16 Jun 2025 07:45:37 -0700 Subject: [PATCH] EXPERIMENTAL - [scheduling] Remove MutualExclusionPass With the new channel multiplexing solution, this is no longer necessary - and any post-scheduling resource sharing will likely use non-Z3-based analyses. DO NOT SUBMIT until we've decided if this is actually a good idea. PiperOrigin-RevId: 772022885 --- xls/codegen/BUILD | 1 + xls/codegen/block_conversion.cc | 15 ++--- xls/codegen/conversion_utils.cc | 68 ++++++++++++++++------ xls/codegen/conversion_utils.h | 10 ++-- xls/codegen/proc_block_conversion.cc | 36 +++++------- xls/contrib/xlscc/unit_tests/unit_test.cc | 1 + xls/scheduling/BUILD | 1 - xls/scheduling/scheduling_pass_pipeline.cc | 6 -- 8 files changed, 76 insertions(+), 62 deletions(-) diff --git a/xls/codegen/BUILD b/xls/codegen/BUILD index 73d3879261..70aad7ad78 100644 --- a/xls/codegen/BUILD +++ b/xls/codegen/BUILD @@ -442,6 +442,7 @@ cc_library( "//xls/ir", "//xls/ir:bits", "//xls/ir:channel", + "//xls/ir:channel_ops", "//xls/ir:node_util", "//xls/ir:op", "//xls/ir:proc_elaboration", diff --git a/xls/codegen/block_conversion.cc b/xls/codegen/block_conversion.cc index 569677ea6a..d3222a36b8 100644 --- a/xls/codegen/block_conversion.cc +++ b/xls/codegen/block_conversion.cc @@ -299,19 +299,15 @@ absl::Status AddCombinationalFlowControl( std::vector>& streaming_outputs, std::vector>& stage_valid, const CodegenOptions& options, Proc* proc, Block* block) { - std::string_view valid_suffix = options.streaming_channel_valid_suffix(); - std::string_view ready_suffix = options.streaming_channel_ready_suffix(); - std::string_view data_suffix = options.streaming_channel_data_suffix(); - XLS_ASSIGN_OR_RETURN( std::vector all_active_outputs_ready, MakeInputReadyPortsForOutputChannels(streaming_outputs, /*stage_count=*/1, - ready_suffix, block)); + options, block)); XLS_ASSIGN_OR_RETURN( std::vector all_active_inputs_valid, MakeInputValidPortsForInputChannels(streaming_inputs, /*stage_count=*/1, - valid_suffix, block)); + options, block)); XLS_ASSIGN_OR_RETURN(Node * literal_1, block->MakeNode( SourceInfo(), Value(UBits(1, 1)))); @@ -319,14 +315,13 @@ absl::Status AddCombinationalFlowControl( std::vector next_stage_open{literal_1}; XLS_RETURN_IF_ERROR(MakeOutputValidPortsForOutputChannels( all_active_inputs_valid, pipelined_valids, next_stage_open, - streaming_outputs, valid_suffix, proc, {}, block)); + streaming_outputs, options, proc, {}, block)); XLS_RETURN_IF_ERROR(MakeOutputDataPortsForOutputChannels( all_active_inputs_valid, pipelined_valids, next_stage_open, - streaming_outputs, data_suffix, block)); + streaming_outputs, options, block)); XLS_RETURN_IF_ERROR(MakeOutputReadyPortsForInputChannels( - all_active_outputs_ready, streaming_inputs, ready_suffix, proc, {}, - block)); + all_active_outputs_ready, streaming_inputs, options, proc, {}, block)); XLS_RET_CHECK(stage_valid.empty()); XLS_RET_CHECK_EQ(all_active_inputs_valid.size(), 1); diff --git a/xls/codegen/conversion_utils.cc b/xls/codegen/conversion_utils.cc index 2bc54a2cb7..1dd5aa0fd4 100644 --- a/xls/codegen/conversion_utils.cc +++ b/xls/codegen/conversion_utils.cc @@ -40,6 +40,7 @@ #include "xls/ir/bits.h" #include "xls/ir/block.h" #include "xls/ir/channel.h" +#include "xls/ir/channel_ops.h" #include "xls/ir/instantiation.h" #include "xls/ir/node.h" #include "xls/ir/node_util.h" @@ -61,32 +62,61 @@ namespace xls::verilog { namespace { +bool HasMultiplexingCycle(const FifoConfig& fifo_config) { + const bool push_to_pop_combo_paths = + fifo_config.bypass() && !fifo_config.register_pop_outputs(); + const bool pop_to_push_combo_paths = !fifo_config.register_push_outputs(); + return push_to_pop_combo_paths && pop_to_push_combo_paths; +} + absl::Status CheckForMultiplexingCycle(Channel* channel, + const CodegenOptions& options, std::string_view proc_name) { if (channel->kind() != ChannelKind::kStreaming) { return absl::OkStatus(); } StreamingChannel* streaming_channel = down_cast(channel); + if (streaming_channel->supported_ops() != ChannelOps::kSendReceive) { + // This is an external channel; any I/O flopping will break the cycle. + if (options.flop_inputs() || options.flop_outputs()) { + return absl::OkStatus(); + } + // If we've been given FIFO config information, we can rely on that; + // otherwise, we should tell the user how to fix the issue. + if (streaming_channel->channel_config().fifo_config().has_value() && + !HasMultiplexingCycle( + *streaming_channel->channel_config().fifo_config())) { + return absl::OkStatus(); + } + return absl::InvalidArgumentError(absl::StrFormat( + "Cannot allow multiple operations from proc `%s` on external streaming " + "channel `%s` due to a potential combinational cycle. This can be " + "fixed by any of the following changes:\n" + "\t1. flop inputs (with the `--flop_inputs` flag),\n" + "\t2. flop outputs (with the `--flop_outputs` flag), or\n" + "if channel `%s` connects to an external FIFO guaranteed to have " + "no combinational path between valid and ready, you can specify the " + "reason for this in the channel's FIFO config; it should involve at " + "least one of registered push outputs, registered pop outputs, or " + "disabled combinational bypass.", + proc_name, ChannelRefName(channel), ChannelRefName(channel))); + } + if (!streaming_channel->channel_config().fifo_config().has_value()) { return absl::InvalidArgumentError( absl::StrCat("Cannot allow multiple operations from proc `", proc_name, "` on streaming channel `", ChannelRefName(channel), "`, which has no known FIFO configuration.")); } - const FifoConfig& fifo_config = - *streaming_channel->channel_config().fifo_config(); - bool push_to_pop_combo_paths = - fifo_config.bypass() && !fifo_config.register_pop_outputs(); - bool pop_to_push_combo_paths = !fifo_config.register_push_outputs(); - if (push_to_pop_combo_paths && pop_to_push_combo_paths) { + if (HasMultiplexingCycle( + *streaming_channel->channel_config().fifo_config())) { return absl::InvalidArgumentError(absl::StrFormat( "Cannot allow multiple operations from proc `%s` on streaming channel " "`%s` due to a combinational cycle. This can be fixed by reconfiguring " "channel `%s` in any of the following ways:\n" - "\t1. use proven-mutually-exclusive channel strictness (if possible),\n" - "\t2. register push outputs,\n" - "\t3. register pop outputs, or\n" - "\t4. disable combinational bypass paths.", + "\t1. register push outputs,\n" + "\t2. register pop outputs, or\n" + "\t3. disable combinational bypass paths.", proc_name, ChannelRefName(channel), ChannelRefName(channel))); } return absl::OkStatus(); @@ -138,7 +168,7 @@ std::string PipelineSignalName(std::string_view root, int64_t stage) { absl::StatusOr> MakeInputReadyPortsForOutputChannels( std::vector>& streaming_outputs, - int64_t stage_count, std::string_view ready_suffix, Block* block) { + int64_t stage_count, const CodegenOptions& options, Block* block) { std::vector result; // Add a ready input port for each streaming output. Gather the ready signals @@ -200,7 +230,7 @@ absl::StatusOr> MakeInputReadyPortsForOutputChannels( // Upon success returns a Node* to the all_active_inputs_valid signal. absl::StatusOr> MakeInputValidPortsForInputChannels( std::vector>& streaming_inputs, - int64_t stage_count, std::string_view valid_suffix, Block* block) { + int64_t stage_count, const CodegenOptions& options, Block* block) { std::vector result; for (Stage stage = 0; stage < stage_count; ++stage) { @@ -424,7 +454,7 @@ absl::Status MakeOutputValidPortsForOutputChannels( absl::Span pipelined_valids, absl::Span next_stage_open, std::vector>& streaming_outputs, - std::string_view valid_suffix, Proc* proc, + const CodegenOptions& options, Proc* proc, absl::Span instances, Block* block) { absl::btree_map>>, @@ -447,13 +477,13 @@ absl::Status MakeOutputValidPortsForOutputChannels( if (std::holds_alternative(channel)) { CHECK(instances.empty()); XLS_RETURN_IF_ERROR(CheckForMultiplexingCycle( - std::get(channel), proc->name())); + std::get(channel), options, proc->name())); } else { CHECK(std::holds_alternative(channel)); CHECK(!instances.empty()); for (ProcInstance* instance : instances) { XLS_RETURN_IF_ERROR(CheckForMultiplexingCycle( - instance->GetChannelBinding(channel).instance->channel, + instance->GetChannelBinding(channel).instance->channel, options, instance->GetName())); } } @@ -553,7 +583,7 @@ absl::Status MakeOutputDataPortsForOutputChannels( absl::Span pipelined_valids, absl::Span next_stage_open, std::vector>& streaming_outputs, - std::string_view data_suffix, Block* block) { + const CodegenOptions& options, Block* block) { absl::btree_map>, Node::NodeIdLessThan> data_values; @@ -613,7 +643,7 @@ absl::Status MakeOutputDataPortsForOutputChannels( absl::Status MakeOutputReadyPortsForInputChannels( absl::Span all_active_outputs_ready, std::vector>& streaming_inputs, - std::string_view ready_suffix, Proc* proc, + const CodegenOptions& options, Proc* proc, absl::Span instances, Block* block) { absl::btree_map>>, @@ -636,13 +666,13 @@ absl::Status MakeOutputReadyPortsForInputChannels( if (std::holds_alternative(channel)) { CHECK(instances.empty()); XLS_RETURN_IF_ERROR(CheckForMultiplexingCycle( - std::get(channel), proc->name())); + std::get(channel), options, proc->name())); } else { CHECK(std::holds_alternative(channel)); CHECK(!instances.empty()); for (ProcInstance* instance : instances) { XLS_RETURN_IF_ERROR(CheckForMultiplexingCycle( - instance->GetChannelBinding(channel).instance->channel, + instance->GetChannelBinding(channel).instance->channel, options, instance->GetName())); } } diff --git a/xls/codegen/conversion_utils.h b/xls/codegen/conversion_utils.h index 291ccbc94c..c18d8d82ec 100644 --- a/xls/codegen/conversion_utils.h +++ b/xls/codegen/conversion_utils.h @@ -59,11 +59,11 @@ std::string PipelineSignalName(std::string_view root, int64_t stage); // Upon success returns a Node* to the all_active_inputs_valid signal. absl::StatusOr> MakeInputReadyPortsForOutputChannels( std::vector>& streaming_outputs, - int64_t stage_count, std::string_view ready_suffix, Block* block); + int64_t stage_count, const CodegenOptions& options, Block* block); absl::StatusOr> MakeInputValidPortsForInputChannels( std::vector>& streaming_inputs, - int64_t stage_count, std::string_view valid_suffix, Block* block); + int64_t stage_count, const CodegenOptions& options, Block* block); // Given a node returns a node that is OR'd with the reset signal. // if said reset signal exists. That node can be thought of as @@ -117,7 +117,7 @@ absl::Status MakeOutputValidPortsForOutputChannels( absl::Span pipelined_valids, absl::Span next_stage_open, std::vector>& streaming_outputs, - std::string_view valid_suffix, Proc* proc, + const CodegenOptions& options, Proc* proc, absl::Span instances, Block* block); // Make data ports (output) for each output channel. @@ -130,7 +130,7 @@ absl::Status MakeOutputDataPortsForOutputChannels( absl::Span pipelined_valids, absl::Span next_stage_open, std::vector>& streaming_outputs, - std::string_view data_suffix, Block* block); + const CodegenOptions& options, Block* block); // Make ready ports (output) for each input channel. // @@ -139,7 +139,7 @@ absl::Status MakeOutputDataPortsForOutputChannels( absl::Status MakeOutputReadyPortsForInputChannels( absl::Span all_active_outputs_ready, std::vector>& streaming_inputs, - std::string_view ready_suffix, Proc* proc, + const CodegenOptions& options, Proc* proc, absl::Span instances, Block* block); } // namespace xls::verilog diff --git a/xls/codegen/proc_block_conversion.cc b/xls/codegen/proc_block_conversion.cc index e790aecf30..8eb3a1e00d 100644 --- a/xls/codegen/proc_block_conversion.cc +++ b/xls/codegen/proc_block_conversion.cc @@ -437,7 +437,7 @@ static absl::StatusOr UpdateSingleStagePipelineWithFlowControl( static absl::StatusOr> MakeValidNodesForInputStates( std::vector>& input_states, absl::Span const> state_registers, - int64_t stage_count, std::string_view valid_suffix, Block* block) { + int64_t stage_count, Block* block) { std::vector result; for (Stage stage = 0; stage < stage_count; ++stage) { // Add a valid signal for each state element with non-trivial backedge. @@ -813,34 +813,28 @@ static absl::Status AddBubbleFlowControl( Proc* proc, absl::Span instances, Block* block, std::vector& all_active_outputs_ready) { int64_t stage_count = streaming_io.pipeline_registers.size() + 1; - std::string_view data_suffix = options.streaming_channel_data_suffix(); - std::string_view valid_suffix = options.streaming_channel_valid_suffix(); - std::string_view ready_suffix = options.streaming_channel_ready_suffix(); // Node in each stage that represents when all inputs channels (that are // predicated true) are valid. - XLS_ASSIGN_OR_RETURN( - std::vector all_active_inputs_valid, - MakeInputValidPortsForInputChannels(streaming_io.inputs, stage_count, - valid_suffix, block)); + XLS_ASSIGN_OR_RETURN(std::vector all_active_inputs_valid, + MakeInputValidPortsForInputChannels( + streaming_io.inputs, stage_count, options, block)); VLOG(3) << "After Inputs Valid"; XLS_VLOG_LINES(3, block->DumpIr()); // Node in each stage that represents when all output channels (that are // predicated true) are ready. - XLS_ASSIGN_OR_RETURN( - all_active_outputs_ready, - MakeInputReadyPortsForOutputChannels(streaming_io.outputs, stage_count, - ready_suffix, block)); + XLS_ASSIGN_OR_RETURN(all_active_outputs_ready, + MakeInputReadyPortsForOutputChannels( + streaming_io.outputs, stage_count, options, block)); VLOG(3) << "After Outputs Ready"; XLS_VLOG_LINES(3, block->DumpIr()); // Node in each stage that represents when all state values are valid. - XLS_ASSIGN_OR_RETURN( - std::vector all_active_states_valid, - MakeValidNodesForInputStates(streaming_io.input_states, - streaming_io.state_registers, stage_count, - valid_suffix, block)); + XLS_ASSIGN_OR_RETURN(std::vector all_active_states_valid, + MakeValidNodesForInputStates( + streaming_io.input_states, + streaming_io.state_registers, stage_count, block)); VLOG(3) << "After States Valid"; XLS_VLOG_LINES(3, block->DumpIr()); @@ -884,11 +878,11 @@ static absl::Status AddBubbleFlowControl( } XLS_RETURN_IF_ERROR(MakeOutputValidPortsForOutputChannels( all_active_inputs_valid, stage_valid_no_option, - bubble_flow_control.next_stage_open, streaming_io.outputs, valid_suffix, + bubble_flow_control.next_stage_open, streaming_io.outputs, options, proc, instances, block)); XLS_RETURN_IF_ERROR(MakeOutputDataPortsForOutputChannels( all_active_inputs_valid, stage_valid_no_option, - bubble_flow_control.next_stage_open, streaming_io.outputs, data_suffix, + bubble_flow_control.next_stage_open, streaming_io.outputs, options, block)); } @@ -910,8 +904,8 @@ static absl::Status AddBubbleFlowControl( XLS_VLOG_LINES(3, block->DumpIr()); XLS_RETURN_IF_ERROR(MakeOutputReadyPortsForInputChannels( - bubble_flow_control.data_load_enable, streaming_io.inputs, ready_suffix, - proc, instances, block)); + bubble_flow_control.data_load_enable, streaming_io.inputs, options, proc, + instances, block)); VLOG(3) << "After Ready"; XLS_VLOG_LINES(3, block->DumpIr()); diff --git a/xls/contrib/xlscc/unit_tests/unit_test.cc b/xls/contrib/xlscc/unit_tests/unit_test.cc index dd0f603492..19cd9a85f2 100644 --- a/xls/contrib/xlscc/unit_tests/unit_test.cc +++ b/xls/contrib/xlscc/unit_tests/unit_test.cc @@ -505,6 +505,7 @@ void XlsccTestBase::BlockTest( codegen_flags_proto.set_generator(xls::GENERATOR_KIND_PIPELINE); codegen_flags_proto.set_streaming_channel_ready_suffix(channel_ready_suffix); codegen_flags_proto.set_streaming_channel_valid_suffix(channel_valid_suffix); + codegen_flags_proto.set_flop_inputs(true); XLS_ASSERT_OK(xls::ScheduleAndCodegen(package_.get(), scheduling_options_flags_proto, diff --git a/xls/scheduling/BUILD b/xls/scheduling/BUILD index 75cca0f5cc..801c60eb0b 100644 --- a/xls/scheduling/BUILD +++ b/xls/scheduling/BUILD @@ -418,7 +418,6 @@ cc_library( srcs = ["scheduling_pass_pipeline.cc"], hdrs = ["scheduling_pass_pipeline.h"], deps = [ - ":mutual_exclusion_pass", ":pipeline_scheduling_pass", ":proc_state_legalization_pass", ":scheduling_checker", diff --git a/xls/scheduling/scheduling_pass_pipeline.cc b/xls/scheduling/scheduling_pass_pipeline.cc index c21568bd70..686c59dde4 100644 --- a/xls/scheduling/scheduling_pass_pipeline.cc +++ b/xls/scheduling/scheduling_pass_pipeline.cc @@ -24,7 +24,6 @@ #include "xls/passes/literal_uncommoning_pass.h" #include "xls/passes/optimization_pass.h" #include "xls/passes/optimization_pass_pipeline.h" -#include "xls/scheduling/mutual_exclusion_pass.h" #include "xls/scheduling/pipeline_scheduling_pass.h" #include "xls/scheduling/proc_state_legalization_pass.h" #include "xls/scheduling/scheduling_checker.h" @@ -44,7 +43,6 @@ std::unique_ptr CreateSchedulingPassPipeline( top->Add(); bool eliminate_noop_next = false; - top->Add(); if (opt_level > 0) { top->Add( std::make_unique(), context, opt_level, @@ -55,10 +53,6 @@ std::unique_ptr CreateSchedulingPassPipeline( top->Add(); top->Add(std::make_unique(), context, opt_level, eliminate_noop_next); - top->Add(); - top->Add(std::make_unique(), - context, opt_level, eliminate_noop_next); - top->Add(); return top; }