Fix reset behavior when codegening side-effecting ops.

Previously, reset was not part of the rewritten condition, so a function with otherwise-illegal inputs during reset could fire an assertion. This change makes reset part of the condition for assertions, traces, and covers.

Also, while we're in here I rewrote the tests to use builders instead of IR.

PiperOrigin-RevId: 713452683

xls/codegen/BUILD

@@ -1602,11 +1602,14 @@ cc_library(
     srcs = ["side_effect_condition_pass.cc"],
     hdrs = ["side_effect_condition_pass.h"],
     deps = [
+        ":block_conversion",
         ":codegen_pass",
         "//xls/common/status:ret_check",
         "//xls/common/status:status_macros",
         "//xls/ir",
+        "//xls/ir:node_util",
         "//xls/ir:op",
+        "//xls/ir:register",
         "//xls/ir:source_location",
         "@com_google_absl//absl/log",
         "@com_google_absl//absl/status",
@@ -1635,9 +1638,13 @@ cc_test(
         "//xls/interpreter:ir_interpreter",
         "//xls/ir",
         "//xls/ir:bits",
+        "//xls/ir:channel_ops",
         "//xls/ir:events",
+        "//xls/ir:function_builder",
         "//xls/ir:ir_matcher",
         "//xls/ir:ir_parser",
+        "//xls/ir:source_location",
+        "//xls/ir:type",
         "//xls/ir:value",
         "//xls/passes:pass_base",
         "//xls/scheduling:pipeline_schedule",

xls/codegen/conversion_utils.cc

@@ -196,8 +196,10 @@ absl::StatusOr<std::vector<Node*>> MakeInputValidPortsForInputChannels(
 //   - Active low reset signals are inverted.
 //
 // See also MakeOrWithResetNode()
-static absl::StatusOr<std::optional<Node*>> MaybeGetOrMakeResetNode(
+absl::StatusOr<std::optional<Node*>> ResetAsserted(
     const std::optional<xls::Reset>& reset_behavior, Block* block) {
+  XLS_RET_CHECK_EQ(block->GetResetPort().has_value(),
+                   reset_behavior.has_value());
   if (!block->GetResetPort().has_value()) {
     return std::nullopt;
   }
@@ -210,6 +212,22 @@ static absl::StatusOr<std::optional<Node*>> MaybeGetOrMakeResetNode(
   return reset_node;
 }
 
+absl::StatusOr<std::optional<Node*>> ResetNotAsserted(
+    const std::optional<xls::Reset>& reset_behavior, Block* block) {
+  XLS_RET_CHECK_EQ(block->GetResetPort().has_value(),
+                   reset_behavior.has_value());
+  if (!block->GetResetPort().has_value()) {
+    return std::nullopt;
+  }
+
+  Node* reset_node = block->GetResetPort().value();
+  if (!reset_behavior->active_low) {
+    return block->MakeNode<UnOp>(/*loc=*/SourceInfo(), reset_node, Op::kNot);
+  }
+
+  return reset_node;
+}
+
 // 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
 //     1 - If being reset or if the src_node is 1
@@ -226,7 +244,7 @@ absl::StatusOr<Node*> MakeOrWithResetNode(
   Node* result = src_node;
 
   XLS_ASSIGN_OR_RETURN(std::optional<Node*> maybe_reset_node,
-                       MaybeGetOrMakeResetNode(reset_behavior, block));
+                       ResetAsserted(reset_behavior, block));
 
   if (maybe_reset_node.has_value()) {
     Node* reset_node = maybe_reset_node.value();

xls/codegen/conversion_utils.h

@@ -60,6 +60,26 @@ absl::StatusOr<Node*> MakeOrWithResetNode(
     Node* src_node, std::string_view result_name,
     const std::optional<xls::Reset>& reset_behavior, Block* block);
 
+// Returns or makes a node that is 1 when the block is under reset, if said
+// reset signal exists.
+//
+//   - If no reset exists, std::nullopt is returned
+//   - Active low reset signals are inverted.
+//
+// See also MakeOrWithResetNode() or ResetNotAsserted().
+absl::StatusOr<std::optional<Node*>> ResetAsserted(
+    const std::optional<xls::Reset>& reset_behavior, Block* block);
+
+// Returns or makes a node that is 1 when the block is not under reset, if said
+// reset signal exists.
+//
+//   - If no reset exists, std::nullopt is returned
+//   - Active high reset signals are inverted.
+//
+// See also ResetAsserted().
+absl::StatusOr<std::optional<Node*>> ResetNotAsserted(
+    const std::optional<xls::Reset>& reset_behavior, Block* block);
+
 // If options specify it, adds and returns an input for a reset signal.
 absl::Status MaybeAddResetPort(Block* block, const CodegenOptions& options);
 

xls/codegen/side_effect_condition_pass.cc

@@ -15,7 +15,6 @@
 #include "xls/codegen/side_effect_condition_pass.h"
 
 #include <cstdint>
-#include <initializer_list>
 #include <memory>
 #include <optional>
 #include <variant>
@@ -28,11 +27,14 @@
 #include "absl/types/span.h"
 #include "absl/types/variant.h"
 #include "xls/codegen/codegen_pass.h"
+#include "xls/codegen/conversion_utils.h"
 #include "xls/common/status/ret_check.h"
 #include "xls/common/status/status_macros.h"
 #include "xls/ir/node.h"
+#include "xls/ir/node_util.h"
 #include "xls/ir/nodes.h"
 #include "xls/ir/op.h"
+#include "xls/ir/register.h"
 #include "xls/ir/source_location.h"
 
 namespace xls::verilog {
@@ -82,25 +84,36 @@ absl::StatusOr<int64_t> GetConditionOperandNumber(Node* node) {
   }
 }
 
-absl::StatusOr<Node*> MakeGuardedConditionForOp(Op op, Node* condition,
-                                                Node* stage_guard,
-                                                Block* block) {
+absl::StatusOr<Node*> MakeGuardedConditionForOp(
+    Op op, Node* condition, Node* stage_guard, Block* block,
+    std::optional<xls::Reset> reset_behavior) {
   XLS_RET_CHECK(stage_guard->GetType()->IsBits() &&
                 stage_guard->GetType()->AsBitsOrDie()->bit_count() == 1);
+  std::optional<Node*> reset_port = block->GetResetPort();
+  XLS_RET_CHECK_EQ(reset_behavior.has_value(), reset_port.has_value());
   switch (op) {
-    case Op::kAssert: {  // Asserts are !stage_guard || condition
+    case Op::kAssert: {  // Asserts are !stage_guard || condition [||
+                         // asserted(reset)]
       XLS_ASSIGN_OR_RETURN(Node * not_stage_guard,
                            block->MakeNode<xls::UnOp>(/*loc=*/SourceInfo(),
                                                       stage_guard, Op::kNot));
-      return block->MakeNode<xls::NaryOp>(
-          /*loc=*/SourceInfo(),
-          std::initializer_list<Node*>{not_stage_guard, condition}, Op::kOr);
+      std::vector<Node*> new_conditions = {not_stage_guard, condition};
+      XLS_ASSIGN_OR_RETURN(std::optional<Node*> reset_asserted,
+                           ResetAsserted(reset_behavior, block));
+      if (reset_asserted.has_value()) {
+        new_conditions.push_back(*reset_asserted);
+      }
+      return NaryOrIfNeeded(block, new_conditions);
     }
     case Op::kCover:    // Cover and trace have the same condition guard:
-    case Op::kTrace: {  // stage_guard && condition
-      return block->MakeNode<xls::NaryOp>(
-          /*loc=*/SourceInfo(),
-          std::initializer_list<Node*>{stage_guard, condition}, Op::kAnd);
+    case Op::kTrace: {  // stage_guard && condition [&& !asserted(reset)]
+      std::vector<Node*> new_conditions = {stage_guard, condition};
+      XLS_ASSIGN_OR_RETURN(std::optional<Node*> reset_not_asserted,
+                           ResetNotAsserted(reset_behavior, block));
+      if (reset_not_asserted.has_value()) {
+        new_conditions.push_back(*reset_not_asserted);
+      }
+      return NaryAndIfNeeded(block, new_conditions);
     }
     default:
       return absl::InvalidArgumentError(absl::StrFormat(
@@ -172,10 +185,11 @@ absl::StatusOr<bool> SideEffectConditionPass::RunInternal(
           "Stage guard not found for stage %d.", condition_stage);
       XLS_ASSIGN_OR_RETURN(int64_t condition_operand,
                            GetConditionOperandNumber(node));
-      XLS_ASSIGN_OR_RETURN(Node * guarded_condition,
-                           MakeGuardedConditionForOp(
-                               node->op(), node->operand(condition_operand),
-                               *stage_guard, block.get()));
+      XLS_ASSIGN_OR_RETURN(
+          Node * guarded_condition,
+          MakeGuardedConditionForOp(
+              node->op(), node->operand(condition_operand), *stage_guard,
+              block.get(), options.codegen_options.ResetBehavior()));
       XLS_RETURN_IF_ERROR(
           node->ReplaceOperandNumber(condition_operand, guarded_condition));
       changed = true;