[codegen 1.5] Add reset requirement for valid-output control

If lowering a function with a valid-output control signal, we should always have a reset signal; otherwise we can end up with (pipeline_depth - 1) cycles of garbage at the valid-output signal, potentially signaling "valid" when the output is very much not valid. This required adding reset signal support to our unit tests' RunFunctionalTest logic for this pass.

While we're at it, we also ensure that the source return value is always available in the last pipeline stage by inserting an identity node if necessary - which can be necessary when lowering a no-op function that directly returns one of its inputs. (This mostly only comes up in testing.)

PiperOrigin-RevId: 853045608

Author: ericastor

xls/codegen_v_1_5/BUILD

@@ -324,12 +324,14 @@ cc_library(
         "//xls/ir:node_util",
         "//xls/ir:op",
         "//xls/ir:register",
+        "//xls/ir:source_location",
         "//xls/ir:value_utils",
         "//xls/passes:pass_base",
         "@com_google_absl//absl/algorithm:container",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
+        "@com_google_absl//absl/strings:str_format",
         "@com_google_absl//absl/types:span",
         "@cppitertools",
     ],

xls/codegen_v_1_5/block_conversion_pass_pipeline_test.cc

@@ -6679,7 +6679,7 @@ TEST_F(BlockConversionTest, ReturnArrayLiteral) {
                                {Value(UBits(0, 1)), Value(UBits(1, 1))}))));
 }
 
-TEST_F(BlockConversionTest, DISABLED_ValidSignalWithoutReset) {
+TEST_F(BlockConversionTest, ValidSignalWithoutReset) {
   Package package(TestName());
   FunctionBuilder fb(TestName(), &package);
   fb.Param("x", package.GetBitsType(8));

xls/codegen_v_1_5/function_io_lowering_pass.cc

@@ -25,6 +25,7 @@
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/str_cat.h"
+#include "absl/strings/str_format.h"
 #include "absl/types/span.h"
 #include "cppitertools/reversed.hpp"
 #include "xls/codegen/conversion_utils.h"
@@ -40,6 +41,7 @@
 #include "xls/ir/op.h"
 #include "xls/ir/package.h"
 #include "xls/ir/register.h"
+#include "xls/ir/source_location.h"
 #include "xls/ir/value_utils.h"
 #include "xls/passes/pass_base.h"
 
@@ -168,16 +170,37 @@ absl::StatusOr<bool> FunctionIOLoweringPass::LowerReturnValue(
   }
 
   // The return value is always assumed to be in the last stage - or possibly
-  // unscheduled, if (e.g.) returning a literal.
-  if (block->IsStaged(block->source_return_value())) {
+  // unscheduled, if (e.g.) returning a literal. If it's not in the last stage,
+  // it must be a Param - so we need to introduce an identity node to ensure
+  // that the value gets piped through the pipeline registers.
+  Node* return_value = block->source_return_value();
+  if (block->IsStaged(return_value)) {
     XLS_ASSIGN_OR_RETURN(int64_t stage_index,
-                         block->GetStageIndex(block->source_return_value()));
-    XLS_RET_CHECK_EQ(stage_index, block->stages().size() - 1);
+                         block->GetStageIndex(return_value));
+    const int64_t last_stage = block->stages().size() - 1;
+    if (stage_index != last_stage) {
+      XLS_ASSIGN_OR_RETURN(
+          return_value,
+          block->MakeNodeInStage<UnOp>(block->stages().size() - 1, SourceInfo(),
+                                       return_value, Op::kIdentity));
+    }
   }
 
   std::optional<Node*> output_valid = std::nullopt;
   if (options.codegen_options.valid_control().has_value() &&
       !options.codegen_options.valid_control()->output_name().empty()) {
+    // If the valid signal is passed all the way through to an output port, then
+    // the block must have a reset port. Otherwise, garbage will be passed out
+    // of the valid out port until the pipeline flushes. If there is not a valid
+    // output port, it's ok for the flopped valid to have garbage values because
+    // it is only used as a term in load enables for power savings.
+    if (!block->GetResetBehavior().has_value()) {
+      return absl::InvalidArgumentError(absl::StrFormat(
+          "Block `%s` has valid signal output but no reset; this would produce "
+          "an incorrect valid-output signal.",
+          block->name()));
+    }
+
     output_valid = block->stages().back().outputs_valid();
     if (options.codegen_options.flop_outputs()) {
       XLS_ASSIGN_OR_RETURN(

xls/codegen_v_1_5/function_io_lowering_pass_test.cc

@@ -130,22 +130,34 @@ class FunctionIOLoweringPassTest : public IrTestBase {
     const int64_t output_cycle =
         options.input_valid_delay + expected_latency - 1;
     in_values.resize(num_cycles);
+    const bool has_reset = pass_options.codegen_options.reset().has_value();
+    const bool has_input_valid = options.input_valid.has_value();
+    const int64_t value_offset =
+        (has_reset ? 1 : 0) + (has_input_valid ? 1 : 0);
     XLS_RET_CHECK_EQ(sim_block->GetInputPorts().size(),
-                     (options.input_valid.has_value() ? 1 : 0) + inputs.size());
+                     value_offset + inputs.size());
     for (int64_t i = 0; i < sim_block->GetInputPorts().size(); ++i) {
       std::string_view input_name = sim_block->GetInputPorts()[i]->name();
-      Value input_value;
-      if (options.input_valid.has_value()) {
-        if (i == 0) {
-          input_value = Value(UBits(1, 1));
-        } else {
-          input_value = inputs[i - 1];
+      if (has_reset && i == 0) {
+        Value reset_disabled = Value(UBits(
+            pass_options.codegen_options.reset()->active_low() ? 1 : 0, 1));
+        for (int64_t j = 0; j < num_cycles; ++j) {
+          in_values[j].emplace(input_name, reset_disabled);
         }
+        continue;
+      }
+
+      Value input_value;
+      if (i < value_offset) {
+        XLS_RET_CHECK(has_input_valid);
+        XLS_RET_CHECK_EQ(i, value_offset - 1);
+        // input_valid should be 1 whenever we have a valid input.
+        input_value = Value(UBits(1, 1));
       } else {
-        input_value = inputs[i];
+        input_value = inputs[i - value_offset];
       }
       for (int64_t j = 0; j <= options.input_valid_delay; ++j) {
-        if (i == 0 && j < options.input_valid_delay) {
+        if (i == value_offset - 1 && j < options.input_valid_delay) {
           in_values[j].emplace(input_name, Value(UBits(0, 1)));
         } else {
           in_values[j].emplace(input_name, input_value);
@@ -285,12 +297,13 @@ TEST_F(FunctionIOLoweringPassTest, SingleStageWithIOValid) {
                            InterpreterResultToStatusOrValue(result));
   ASSERT_EQ(expected_output, Value(UBits(42, 32)));
 
-  XLS_ASSERT_OK_AND_ASSIGN(
-      BlockConversionPassOptions options,
-      CreateBlockConversionPassOptions(
-          p.get(), /*pipeline_stages=*/1,
-          ::xls::verilog::CodegenOptions().clock_name("clk").valid_control(
-              "x_valid", "out_valid")));
+  XLS_ASSERT_OK_AND_ASSIGN(BlockConversionPassOptions options,
+                           CreateBlockConversionPassOptions(
+                               p.get(), /*pipeline_stages=*/1,
+                               ::xls::verilog::CodegenOptions()
+                                   .clock_name("clk")
+                                   .reset("rst", false, false, false)
+                                   .valid_control("x_valid", "out_valid")));
   XLS_ASSERT_OK_AND_ASSIGN(ScheduledBlock * sb,
                            CreateScheduledBlock(p.get(), "id_func", options));
 
@@ -299,7 +312,8 @@ TEST_F(FunctionIOLoweringPassTest, SingleStageWithIOValid) {
               IsOkAndHolds(true));
 
   EXPECT_THAT(sb->GetInputPorts(),
-              ElementsAre(Property(&PortNode::GetName, "x_valid"),
+              ElementsAre(Property(&PortNode::GetName, "rst"),
+                          Property(&PortNode::GetName, "x_valid"),
                           Property(&PortNode::GetName, "x")));
   EXPECT_THAT(sb->GetOutputPorts(),
               ElementsAre(Property(&PortNode::GetName, "out_valid"),
@@ -420,12 +434,13 @@ TEST_F(FunctionIOLoweringPassTest, MultiStageWithIOValidDelayed) {
                            InterpreterResultToStatusOrValue(result));
   ASSERT_EQ(expected_output, Value(UBits(42, 32)));
 
-  XLS_ASSERT_OK_AND_ASSIGN(
-      BlockConversionPassOptions options,
-      CreateBlockConversionPassOptions(
-          p.get(), /*pipeline_stages=*/3,
-          ::xls::verilog::CodegenOptions().clock_name("clk").valid_control(
-              "x_valid", "out_valid")));
+  XLS_ASSERT_OK_AND_ASSIGN(BlockConversionPassOptions options,
+                           CreateBlockConversionPassOptions(
+                               p.get(), /*pipeline_stages=*/3,
+                               ::xls::verilog::CodegenOptions()
+                                   .clock_name("clk")
+                                   .reset("rst", false, false, false)
+                                   .valid_control("x_valid", "out_valid")));
   XLS_ASSERT_OK_AND_ASSIGN(ScheduledBlock * sb,
                            CreateScheduledBlock(p.get(), "id_func", options));
 
@@ -434,7 +449,8 @@ TEST_F(FunctionIOLoweringPassTest, MultiStageWithIOValidDelayed) {
               IsOkAndHolds(true));
 
   EXPECT_THAT(sb->GetInputPorts(),
-              ElementsAre(Property(&PortNode::GetName, "x_valid"),
+              ElementsAre(Property(&PortNode::GetName, "rst"),
+                          Property(&PortNode::GetName, "x_valid"),
                           Property(&PortNode::GetName, "x")));
   EXPECT_THAT(sb->GetOutputPorts(),
               ElementsAre(Property(&PortNode::GetName, "out_valid"),
@@ -469,12 +485,13 @@ TEST_F(FunctionIOLoweringPassTest, MultiStageWithIOValid) {
                            InterpreterResultToStatusOrValue(result));
   ASSERT_EQ(expected_output, Value(UBits(42, 32)));
 
-  XLS_ASSERT_OK_AND_ASSIGN(
-      BlockConversionPassOptions options,
-      CreateBlockConversionPassOptions(
-          p.get(), /*pipeline_stages=*/3,
-          ::xls::verilog::CodegenOptions().clock_name("clk").valid_control(
-              "x_valid", "out_valid")));
+  XLS_ASSERT_OK_AND_ASSIGN(BlockConversionPassOptions options,
+                           CreateBlockConversionPassOptions(
+                               p.get(), /*pipeline_stages=*/3,
+                               ::xls::verilog::CodegenOptions()
+                                   .clock_name("clk")
+                                   .reset("rst", false, false, false)
+                                   .valid_control("x_valid", "out_valid")));
   XLS_ASSERT_OK_AND_ASSIGN(ScheduledBlock * sb,
                            CreateScheduledBlock(p.get(), "id_func", options));
 
@@ -483,7 +500,8 @@ TEST_F(FunctionIOLoweringPassTest, MultiStageWithIOValid) {
               IsOkAndHolds(true));
 
   EXPECT_THAT(sb->GetInputPorts(),
-              ElementsAre(Property(&PortNode::GetName, "x_valid"),
+              ElementsAre(Property(&PortNode::GetName, "rst"),
+                          Property(&PortNode::GetName, "x_valid"),
                           Property(&PortNode::GetName, "x")));
   EXPECT_THAT(sb->GetOutputPorts(),
               ElementsAre(Property(&PortNode::GetName, "out_valid"),
@@ -517,12 +535,13 @@ TEST_F(FunctionIOLoweringPassTest, MultiStageMultiInputWithIOValid) {
                            InterpreterResultToStatusOrValue(result));
   ASSERT_EQ(expected_output, Value(UBits(63, 32)));
 
-  XLS_ASSERT_OK_AND_ASSIGN(
-      BlockConversionPassOptions options,
-      CreateBlockConversionPassOptions(
-          p.get(), /*pipeline_stages=*/3,
-          ::xls::verilog::CodegenOptions().clock_name("clk").valid_control(
-              "in_valid", "out_valid")));
+  XLS_ASSERT_OK_AND_ASSIGN(BlockConversionPassOptions options,
+                           CreateBlockConversionPassOptions(
+                               p.get(), /*pipeline_stages=*/3,
+                               ::xls::verilog::CodegenOptions()
+                                   .clock_name("clk")
+                                   .reset("rst", false, false, false)
+                                   .valid_control("in_valid", "out_valid")));
   XLS_ASSERT_OK_AND_ASSIGN(ScheduledBlock * sb,
                            CreateScheduledBlock(p.get(), "id_func", options));
 
@@ -531,7 +550,8 @@ TEST_F(FunctionIOLoweringPassTest, MultiStageMultiInputWithIOValid) {
               IsOkAndHolds(true));
 
   EXPECT_THAT(sb->GetInputPorts(),
-              ElementsAre(Property(&PortNode::GetName, "in_valid"),
+              ElementsAre(Property(&PortNode::GetName, "rst"),
+                          Property(&PortNode::GetName, "in_valid"),
                           Property(&PortNode::GetName, "x"),
                           Property(&PortNode::GetName, "y")));
   EXPECT_THAT(sb->GetOutputPorts(),