Replace zero-latency buffer with FIFO instantiation in RAM rewrite pass.

xls/codegen/BUILD

@@ -1721,6 +1721,7 @@ cc_test(
         "//xls/ir:channel",
         "//xls/ir:ir_matcher",
         "//xls/ir:ir_test_base",
+        "//xls/ir:op",
         "//xls/ir:verifier",
         "//xls/passes:optimization_pass",
         "//xls/passes:pass_base",

xls/codegen/ram_rewrite_pass.cc

@@ -40,6 +40,7 @@
 #include "xls/ir/bits.h"
 #include "xls/ir/block.h"
 #include "xls/ir/channel.h"
+#include "xls/ir/instantiation.h"
 #include "xls/ir/node.h"
 #include "xls/ir/nodes.h"
 #include "xls/ir/op.h"
@@ -53,6 +54,15 @@ namespace xls::verilog {
 
 namespace {
 
+// We internalize the channel flow control by adding a FIFO that catches the
+// response data from the SRAM and drives the flow control of the
+// request/response channels. For now, we do not provide a mechanism to
+// configure this FIFO.
+// TODO: google/xls#1053 - Provide a mechanism to configure the internal FIFOs.
+constexpr FifoConfig kResponseChannelFifoConfig(/*depth=*/1, /*bypass=*/true,
+                                                /*register_push_outputs=*/false,
+                                                /*register_pop_inputs=*/false);
+
 struct ReqBlockPorts {
   OutputPort* req_data;
   OutputPort* req_valid;
@@ -179,6 +189,66 @@ absl::StatusOr<RespBlockPorts> GetRespBlockPorts(
                         .resp_ready = resp_ready_port};
 }
 
+absl::Status AddFifo(Node* from_data, Node* from_valid, Node* from_rdy,
+                     std::string_view name_prefix, Block* block,
+                     const FifoConfig& fifo_config,
+                     std::vector<std::optional<Node*>>& valid_nodes) {
+  XLS_ASSIGN_OR_RETURN(
+      FifoInstantiation * fifo_instantiation,
+      block->AddFifoInstantiation(absl::StrCat(name_prefix, "_fifo"),
+                                  fifo_config, from_data->GetType()));
+  XLS_RET_CHECK(block->GetResetPort().has_value() &&
+                block->GetResetBehavior().has_value());
+  Node* corrected_reset = *block->GetResetPort();
+  // TODO: google/xls#1391 - the fifo implementation should have a way to
+  // express its reset behavior, so we should eventually not assume the fifo's
+  // reset is active-high.
+  if (block->GetResetBehavior()->active_low) {
+    XLS_ASSIGN_OR_RETURN(
+        corrected_reset,
+        block->MakeNode<UnOp>(from_data->loc(), corrected_reset, Op::kNot));
+  }
+  XLS_RETURN_IF_ERROR(
+      block
+          ->MakeNode<InstantiationInput>(from_data->loc(), corrected_reset,
+                                         fifo_instantiation,
+                                         FifoInstantiation::kResetPortName)
+          .status());
+  XLS_RETURN_IF_ERROR(
+      from_data
+          ->ReplaceUsesWithNew<InstantiationOutput>(
+              fifo_instantiation, FifoInstantiation::kPopDataPortName)
+          .status());
+  XLS_RETURN_IF_ERROR(
+      from_valid
+          ->ReplaceUsesWithNew<InstantiationOutput>(
+              fifo_instantiation, FifoInstantiation::kPopValidPortName)
+          .status());
+  XLS_RETURN_IF_ERROR(
+      from_rdy
+          ->ReplaceUsesWithNew<InstantiationOutput>(
+              fifo_instantiation, FifoInstantiation::kPushReadyPortName)
+          .status());
+
+  XLS_RETURN_IF_ERROR(block
+                          ->MakeNode<InstantiationInput>(
+                              from_data->loc(), from_data, fifo_instantiation,
+                              FifoInstantiation::kPushDataPortName)
+                          .status());
+  XLS_RETURN_IF_ERROR(block
+                          ->MakeNode<InstantiationInput>(
+                              from_data->loc(), from_valid, fifo_instantiation,
+                              FifoInstantiation::kPushValidPortName)
+                          .status());
+  XLS_RETURN_IF_ERROR(block
+                          ->MakeNode<InstantiationInput>(
+                              from_data->loc(), from_rdy, fifo_instantiation,
+                              FifoInstantiation::kPopReadyPortName)
+                          .status());
+
+  return absl::OkStatus();
+}
+
 absl::StatusOr<RamRWPortBlockPorts> GetRWBlockPorts(
     Block* const block, const RamRWPortConfiguration& port_config) {
   RamRWPortBlockPorts ports;
@@ -492,15 +562,14 @@ absl::StatusOr<bool> Ram1RWRewrite(Package* package,
   XLS_RETURN_IF_ERROR(
       rw_block_ports.req_ports.req_ready->ReplaceUsesWith(resp_ready_port_buf));
 
-  // Add zero-latency buffer at output of ram
+  // Add fifo at output of ram- the idea is to use the fifo to manage the flow
+  // control in the event that the req or resp side stalls.
   std::vector<std::optional<Node*>> valid_nodes;
   std::string zero_latency_buffer_name =
       absl::StrCat(ram_name, "_ram_zero_latency0");
-  XLS_RETURN_IF_ERROR(AddZeroLatencyBufferToRDVNodes(
-                          resp_rd_data_port, ram_resp_valid,
-                          resp_ready_port_buf, zero_latency_buffer_name, block,
-                          valid_nodes)
-                          .status());
+  XLS_RETURN_IF_ERROR(AddFifo(resp_rd_data_port, ram_resp_valid,
+                              resp_ready_port_buf, zero_latency_buffer_name,
+                              block, kResponseChannelFifoConfig, valid_nodes));
 
   // Add output ports for expanded req.data.
   XLS_ASSIGN_OR_RETURN(auto* req_addr_port,
@@ -677,10 +746,11 @@ absl::StatusOr<bool> Ram1R1WRewrite(Package* package,
   std::vector<std::optional<Node*>> valid_nodes;
   std::string zero_latency_buffer_name =
       absl::StrCat(ram_name, "_ram_zero_latency0");
-  XLS_RETURN_IF_ERROR(AddZeroLatencyBufferToRDVNodes(
-                          rd_data_port, rd_resp_valid, resp_ready_port_buf,
-                          zero_latency_buffer_name, block, valid_nodes)
-                          .status());
+  // Add fifo at output of ram- the idea is to use the fifo to manage the flow
+  // control in the event that the req or resp side stalls.
+  XLS_RETURN_IF_ERROR(AddFifo(rd_data_port, rd_resp_valid, resp_ready_port_buf,
+                              zero_latency_buffer_name, block,
+                              kResponseChannelFifoConfig, valid_nodes));
   // Replace write ready with literal 1 (RAM is always ready for write).
   // TODO(rigge): should this signal check for hazards?
   XLS_ASSIGN_OR_RETURN(

xls/codegen/ram_rewrite_pass_test.cc

@@ -53,9 +53,11 @@
 #include "xls/estimators/delay_model/delay_estimators.h"
 #include "xls/ir/block.h"
 #include "xls/ir/channel.h"
+#include "xls/ir/instantiation.h"
 #include "xls/ir/ir_matcher.h"
 #include "xls/ir/ir_test_base.h"
 #include "xls/ir/nodes.h"
+#include "xls/ir/op.h"
 #include "xls/ir/package.h"
 #include "xls/ir/proc.h"
 #include "xls/ir/verifier.h"
@@ -1533,6 +1535,178 @@ TEST(RamRewritePassInvalidInputsTest, ReadResponseElementNotBits1R1W) {
                                  "contain token, got token.")));
 }
 
+// Helper function to create a 1RW block and run codegen pass with custom
+// active_low reset.
+absl::StatusOr<Block*> MakeBlockAndRunPassesWithReset(Package* package,
+                                                      std::string_view ram_kind,
+                                                      bool active_low) {
+  std::vector<RamConfiguration> ram_configurations;
+  if (ram_kind == "1RW") {
+    ram_configurations.push_back(
+        Ram1RWConfiguration("ram", 1, "req", "resp", "wr_comp"));
+  } else {
+    return absl::InvalidArgumentError("Only 1RW is supported for this helper");
+  }
+
+  const CodegenOptions codegen_options =
+      CodegenOptions()
+          .flop_inputs(false)
+          .flop_outputs(false)
+          .clock_name("clk")
+          .reset("rst", false, active_low, false)
+          .streaming_channel_data_suffix("_data")
+          .streaming_channel_valid_suffix("_valid")
+          .streaming_channel_ready_suffix("_ready")
+          .module_name("pipelined_proc")
+          .ram_configurations(ram_configurations);
+  const CodegenPassOptions pass_options{
+      .codegen_options = codegen_options,
+  };
+
+  XLS_ASSIGN_OR_RETURN(Proc * proc, package->GetProc("my_proc"));
+
+  auto scheduling_options = SchedulingOptions();
+  scheduling_options.pipeline_stages(2)
+      .add_constraint(
+          IOConstraint("req", IODirection::kSend, "resp", IODirection::kReceive,
+                       /*minimum_latency=*/1, /*maximum_latency=*/1))
+      .add_constraint(IOConstraint(
+          "req", IODirection::kSend, "wr_comp", IODirection::kReceive,
+          /*minimum_latency=*/1, /*maximum_latency=*/1));
+
+  XLS_ASSIGN_OR_RETURN(auto delay_estimator, GetDelayEstimator("unit"));
+  XLS_ASSIGN_OR_RETURN(
+      PipelineSchedule schedule,
+      RunPipelineSchedule(proc, *delay_estimator, scheduling_options));
+  XLS_ASSIGN_OR_RETURN(
+      CodegenContext context,
+      FunctionBaseToPipelinedBlock(schedule, codegen_options, proc));
+  OptimizationContext opt_context;
+  XLS_RET_CHECK_OK(
+      RunCodegenPassPipeline(pass_options, context.top_block(), opt_context));
+  return context.top_block();
+}
+
+TEST(RamRewritePassFifoTest, FifoHasRightConfigAndPorts) {
+  std::string ir_text = MakeTestProc1RW({});
+  XLS_ASSERT_OK_AND_ASSIGN(auto package, IrTestBase::ParsePackage(ir_text));
+  XLS_ASSERT_OK_AND_ASSIGN(
+      Block * block, MakeBlockAndRunPasses(package.get(), /*ram_kind=*/"1RW"));
+
+  // Retrieve the FIFO instantiation.
+  absl::Span<Instantiation* const> instantiations = block->GetInstantiations();
+  ASSERT_EQ(instantiations.size(), 1);
+
+  XLS_ASSERT_OK_AND_ASSIGN(FifoInstantiation * fifo_instantiation,
+                           instantiations[0]->AsFifoInstantiation());
+
+  EXPECT_EQ(fifo_instantiation->fifo_config().depth(), 1);
+  EXPECT_TRUE(fifo_instantiation->fifo_config().bypass());
+  EXPECT_FALSE(fifo_instantiation->fifo_config().register_push_outputs());
+  EXPECT_FALSE(fifo_instantiation->fifo_config().register_pop_outputs());
+}
+
+TEST(RamRewritePassFifoTest, FifoRouting1RW) {
+  std::string ir_text = MakeTestProc1RW({});
+  XLS_ASSERT_OK_AND_ASSIGN(auto package, IrTestBase::ParsePackage(ir_text));
+  XLS_ASSERT_OK_AND_ASSIGN(
+      Block * block, MakeBlockAndRunPasses(package.get(), /*ram_kind=*/"1RW"));
+
+  absl::Span<Instantiation* const> instantiations = block->GetInstantiations();
+  ASSERT_EQ(instantiations.size(), 1);
+  Instantiation* fifo = instantiations[0];
+
+  // Check inputs into the FIFO
+  XLS_ASSERT_OK_AND_ASSIGN(
+      InstantiationInput * push_data_in,
+      block->GetInstantiationInput(fifo, FifoInstantiation::kPushDataPortName));
+  XLS_ASSERT_OK_AND_ASSIGN(InstantiationInput * push_valid_in,
+                           block->GetInstantiationInput(
+                               fifo, FifoInstantiation::kPushValidPortName));
+  XLS_ASSERT_OK_AND_ASSIGN(
+      InstantiationInput * pop_ready_in,
+      block->GetInstantiationInput(fifo, FifoInstantiation::kPopReadyPortName));
+  XLS_ASSERT_OK_AND_ASSIGN(
+      InstantiationInput * rst_in,
+      block->GetInstantiationInput(fifo, FifoInstantiation::kResetPortName));
+
+  // The data signal driving kPushDataPortName should be the RAM read data input
+  // port.
+  EXPECT_EQ(push_data_in->operand(0)->op(), Op::kInputPort);
+  EXPECT_EQ(push_data_in->operand(0)->As<InputPort>()->name(), "ram_rd_data");
+
+  // The valid signal driving kPushValidPortName should be a register read.
+  EXPECT_EQ(push_valid_in->operand(0)->op(), Op::kRegisterRead);
+
+  // The ready signal driving kPopReadyPortName should be the folded logic.
+  EXPECT_EQ(pop_ready_in->operand(0)->op(), Op::kAnd);
+
+  // The rst signal driving kResetPortName should be the block's reset input
+  // port.
+  EXPECT_EQ(rst_in->operand(0)->op(), Op::kInputPort);
+  EXPECT_EQ(rst_in->operand(0)->As<InputPort>()->name(), "rst");
+
+  // Check outputs from the FIFO
+  XLS_ASSERT_OK_AND_ASSIGN(
+      InstantiationOutput * pop_data_out,
+      block->GetInstantiationOutput(fifo, FifoInstantiation::kPopDataPortName));
+  XLS_ASSERT_OK_AND_ASSIGN(InstantiationOutput * pop_valid_out,
+                           block->GetInstantiationOutput(
+                               fifo, FifoInstantiation::kPopValidPortName));
+  XLS_ASSERT_OK_AND_ASSIGN(InstantiationOutput * push_ready_out,
+                           block->GetInstantiationOutput(
+                               fifo, FifoInstantiation::kPushReadyPortName));
+
+  EXPECT_TRUE(pop_data_out->GetType()->IsBits());
+  EXPECT_TRUE(pop_valid_out->GetType()->IsBits());
+  EXPECT_TRUE(push_ready_out->GetType()->IsBits());
+}
+
+TEST(RamRewritePassFifoTest, ResetCorrectedIfActiveLow) {
+  std::string ir_text = MakeTestProc1RW({});
+  XLS_ASSERT_OK_AND_ASSIGN(auto package, IrTestBase::ParsePackage(ir_text));
+  XLS_ASSERT_OK_AND_ASSIGN(Block * block,
+                           MakeBlockAndRunPassesWithReset(package.get(), "1RW",
+                                                          /*active_low=*/true));
+
+  absl::Span<Instantiation* const> instantiations = block->GetInstantiations();
+  ASSERT_EQ(instantiations.size(), 1);
+  Instantiation* fifo = instantiations[0];
+
+  XLS_ASSERT_OK_AND_ASSIGN(
+      InstantiationInput * rst_in,
+      block->GetInstantiationInput(fifo, FifoInstantiation::kResetPortName));
+
+  // Since active_low is true, the reset node to FIFO should be negated
+  // (Op::kNot) of block's reset input port.
+  Node* reset_operand = rst_in->operand(0);
+  EXPECT_EQ(reset_operand->op(), Op::kNot);
+  EXPECT_EQ(reset_operand->operand(0)->op(), Op::kInputPort);
+  EXPECT_EQ(reset_operand->operand(0)->As<InputPort>()->name(), "rst");
+}
+
+TEST(RamRewritePassFifoTest, ResetUnmodifiedIfActiveHigh) {
+  std::string ir_text = MakeTestProc1RW({});
+  XLS_ASSERT_OK_AND_ASSIGN(auto package, IrTestBase::ParsePackage(ir_text));
+  XLS_ASSERT_OK_AND_ASSIGN(
+      Block * block, MakeBlockAndRunPassesWithReset(package.get(), "1RW",
+                                                    /*active_low=*/false));
+
+  absl::Span<Instantiation* const> instantiations = block->GetInstantiations();
+  ASSERT_EQ(instantiations.size(), 1);
+  Instantiation* fifo = instantiations[0];
+
+  XLS_ASSERT_OK_AND_ASSIGN(
+      InstantiationInput * rst_in,
+      block->GetInstantiationInput(fifo, FifoInstantiation::kResetPortName));
+
+  // Since active_low is false, the reset node to FIFO should be exactly the
+  // block's reset input port.
+  Node* reset_operand = rst_in->operand(0);
+  EXPECT_EQ(reset_operand->op(), Op::kInputPort);
+  EXPECT_EQ(reset_operand->As<InputPort>()->name(), "rst");
+}
+
 }  // namespace
 }  // namespace verilog
 }  // namespace xls