Remove codegen specialization for priority selects.

We previously specialized the codegen of priority selects when the selector was never zero. The specialization would call `$error()` if the selector was zero and propagate `X`, the idea being that this form might synthesize to something smaller.

Unfortunately, this specialization causes more headaches than it is worth. The `$error()` call isn't a real assertion, so it could spuriously fire. We also generally prefer to avoid propagating X explicitly.

Benchmarks showed that this specialization didn't give a net benefit on our corpus, so it seems better to avoid the headaches by removing it.

Fixes #1481.

PiperOrigin-RevId: 742428399

Author: grebe

xls/codegen/combinational_generator_test.cc

@@ -513,47 +513,6 @@ top fn main(p: bits[2], x: bits[16], y: bits[16], d: bits[16]) -> bits[16] {
               IsOkAndHolds(Value(UBits(0xf1ef, 16))));
 }
 
-TEST_P(CombinationalGeneratorTest, PrioritySelectSelectorNeverZero) {
-  std::string text = R"(
-package PrioritySelectSelectorNeverZero
-
-top fn main(p: bits[2], x: bits[16], y: bits[16], d: bits[16]) -> bits[16] {
-  literal.1: bits[2] = literal(value=1)
-  or.2: bits[2] = or(p, literal.1)
-  ret priority_sel.3: bits[16] = priority_sel(or.2, cases=[x, y], default=d)
-}
-)";
-  XLS_ASSERT_OK_AND_ASSIGN(std::unique_ptr<Package> package,
-                           Parser::ParsePackage(text));
-
-  std::optional<FunctionBase*> top = package->GetTop();
-  ASSERT_TRUE(top.has_value());
-  XLS_ASSERT_OK_AND_ASSIGN(
-      auto result, GenerateCombinationalModule(top.value(), codegen_options()));
-
-  ExpectVerilogEqualToGoldenFile(GoldenFilePath(kTestName, kTestdataPath),
-                                 result.verilog_text);
-
-  ModuleSimulator simulator =
-      NewModuleSimulator(result.verilog_text, result.signature);
-  absl::flat_hash_map<std::string, Value> args = {
-      {"x", Value(UBits(0x00ff, 16))},
-      {"y", Value(UBits(0xf0f0, 16))},
-      {"d", Value(UBits(0xff00, 16))}};
-  args["p"] = Value(UBits(0b00, 2));
-  EXPECT_THAT(simulator.RunFunction(args),
-              IsOkAndHolds(Value(UBits(0x00ff, 16))));
-  args["p"] = Value(UBits(0b01, 2));
-  EXPECT_THAT(simulator.RunFunction(args),
-              IsOkAndHolds(Value(UBits(0x00ff, 16))));
-  args["p"] = Value(UBits(0b10, 2));
-  EXPECT_THAT(simulator.RunFunction(args),
-              IsOkAndHolds(Value(UBits(0x00ff, 16))));
-  args["p"] = Value(UBits(0b11, 2));
-  EXPECT_THAT(simulator.RunFunction(args),
-              IsOkAndHolds(Value(UBits(0x00ff, 16))));
-}
-
 TEST_P(CombinationalGeneratorTest, PrioritySelectWithAndWithoutDefault) {
   // Tests that the different specialized ways of codegen'ing priority selects
   // are applied in the right context, e.g. we don't make a no-default priority
@@ -734,48 +693,6 @@ top fn main(p: bits[2], x_short: bits[16][2], x_long: bits[16][4], y_short: bits
               IsOkAndHolds(Value::Tuple({x_short_value, x_long_value})));
 }
 
-TEST_P(CombinationalGeneratorTest, PrioritySelectArraySelectorNeverZero) {
-  std::string text = R"(
-package PrioritySelectArraySelectorNeverZero
-
-top fn main(p: bits[2], x: bits[16][4], y: bits[16][4], d: bits[16][4]) -> bits[16][4] {
-
-  literal.1: bits[2] = literal(value=1)
-  or.2: bits[2] = or(p, literal.1)
-  ret priority_sel.3: bits[16][4] = priority_sel(or.2, cases=[x, y], default=d)
-}
-)";
-  XLS_ASSERT_OK_AND_ASSIGN(std::unique_ptr<Package> package,
-                           Parser::ParsePackage(text));
-
-  std::optional<FunctionBase*> top = package->GetTop();
-  ASSERT_TRUE(top.has_value());
-  XLS_ASSERT_OK_AND_ASSIGN(
-      auto result, GenerateCombinationalModule(top.value(), codegen_options()));
-
-  ExpectVerilogEqualToGoldenFile(GoldenFilePath(kTestName, kTestdataPath),
-                                 result.verilog_text);
-
-  ModuleSimulator simulator =
-      NewModuleSimulator(result.verilog_text, result.signature);
-  XLS_ASSERT_OK_AND_ASSIGN(
-      Value x_value, Value::UBitsArray({0x00ff, 0x00ff, 0x00ff, 0x00ff}, 16));
-  XLS_ASSERT_OK_AND_ASSIGN(
-      Value y_value, Value::UBitsArray({0xf0f0, 0xf0f0, 0xf0f0, 0xf0f0}, 16));
-  XLS_ASSERT_OK_AND_ASSIGN(
-      Value d_value, Value::UBitsArray({0xff00, 0xff00, 0xff00, 0xff00}, 16));
-  absl::flat_hash_map<std::string, Value> args = {
-      {"x", x_value}, {"y", y_value}, {"d", d_value}};
-  args["p"] = Value(UBits(0b00, 2));
-  EXPECT_THAT(simulator.RunFunction(args), IsOkAndHolds(x_value));
-  args["p"] = Value(UBits(0b01, 2));
-  EXPECT_THAT(simulator.RunFunction(args), IsOkAndHolds(x_value));
-  args["p"] = Value(UBits(0b10, 2));
-  EXPECT_THAT(simulator.RunFunction(args), IsOkAndHolds(x_value));
-  args["p"] = Value(UBits(0b11, 2));
-  EXPECT_THAT(simulator.RunFunction(args), IsOkAndHolds(x_value));
-}
-
 TEST_P(CombinationalGeneratorTest, PrioritySelectArrayOneHot) {
   std::string text = R"(
 package PrioritySelectArrayOneHot

xls/codegen/module_builder.cc

@@ -166,7 +166,7 @@ absl::StatusOr<ArrayAssignmentPattern*> ValueToArrayAssignmentPattern(
 absl::StatusOr<VerilogFunction*> DefinePrioritySelectFunction(
     Node* selector, Type* tpe, int64_t num_cases, const SourceInfo& loc,
     std::string_view function_name, ModuleSection* section,
-    SelectorProperties selector_properties, const CodegenOptions& options) {
+    const CodegenOptions& options) {
   VerilogFile* file = section->file();
 
   VerilogFunction* func = section->Add<VerilogFunction>(
@@ -228,32 +228,8 @@ absl::StatusOr<VerilogFunction*> DefinePrioritySelectFunction(
     // Verilog doesn't support 'X, so use the less desirable 16'dxxxx format.
     x_literal = file->Make<XSentinel>(loc, tpe->GetFlatBitCount());
   }
-  if (selector_properties.never_zero) {
-    // If the selector cannot be zero, throw an error if we see zero.
-    // We still explicitly propagate X (like the default case below) for
-    // synthesis.
-    // TODO: github/xls#1481 - this should really be an assert (or assume)
-    // predicated on selector being valid, but it's a bit tricky to do. For now,
-    // it seems better to have an overactive error rather than silently
-    // propagate X.
-    Expression* error_message =
-        file->Make<QuotedString>(loc, "Zero selector not allowed.");
-    StatementBlock* case_block = case_statement->AddCaseArm(zero_label);
-    auto [macro_name, polarity] =
-        MacroNameAndPolarity(options.simulation_macro_name());
-    MacroStatementBlock* ifdef_block =
-        case_block
-            ->Add<StatementConditionalDirective>(loc, polarity, macro_name)
-            ->consequent();
-    ifdef_block->Add<SystemTaskCall>(
-        loc, "error", std::initializer_list<Expression*>{error_message});
-    case_block->Add<Comment>(loc, "Never taken, propagate X");
-    case_block->Add<BlockingAssignment>(loc, func->return_value_ref(),
-                                        x_literal);
-  } else {
-    case_statement->AddCaseArm(zero_label)
-        ->Add<BlockingAssignment>(loc, func->return_value_ref(), default_value);
-  }
+  case_statement->AddCaseArm(zero_label)
+      ->Add<BlockingAssignment>(loc, func->return_value_ref(), default_value);
 
   // Add a default case that propagates X.
   StatementBlock* case_block = case_statement->AddCaseArm(DefaultSentinel());
@@ -798,15 +774,6 @@ absl::Status ModuleBuilder::EmitArrayCopyAndUpdate(
   return absl::OkStatus();
 }
 
-absl::StatusOr<SelectorProperties> ModuleBuilder::GetSelectorProperties(
-    Node* selector) {
-  XLS_RET_CHECK(selector->GetType()->IsBits());
-  XLS_RETURN_IF_ERROR(
-      query_engine_.Populate(selector->function_base()).status());
-  return SelectorProperties{.never_zero =
-                                query_engine_.AtLeastOneBitTrue(selector)};
-}
-
 // We emit asserts if at least one of the following is true:
 //   (1) We are targeting SystemVerilog. In this case, we have a default
 //   assertion format.
@@ -1138,16 +1105,13 @@ absl::StatusOr<LogicRef*> ModuleBuilder::EmitAsAssignment(
         if (node_functions_.contains(function_name)) {
           func = node_functions_.at(function_name);
         } else {
-          XLS_ASSIGN_OR_RETURN(
-              SelectorProperties selector_properties,
-              GetSelectorProperties(node->As<PrioritySelect>()->selector()));
           XLS_ASSIGN_OR_RETURN(
               func,
               DefinePrioritySelectFunction(
                   node->As<PrioritySelect>()->selector(), /*tpe=*/element_type,
                   /*num_cases=*/node->As<PrioritySelect>()->cases().size(),
                   /*loc=*/node->loc(), function_name, functions_section_,
-                  selector_properties, options_));
+                  options_));
           node_functions_[function_name] = func;
         }
         auto priority_sel_element = [&](absl::Span<Expression* const> inputs) {
@@ -1550,16 +1514,9 @@ absl::StatusOr<std::string> ModuleBuilder::VerilogFunctionName(Node* node) {
           "%s_w%d_%db_%db", OpToString(node->op()), node->BitCountOrDie(),
           node->operand(1)->BitCountOrDie(), node->operand(2)->BitCountOrDie());
     case Op::kPrioritySel: {
-      XLS_ASSIGN_OR_RETURN(
-          SelectorProperties selector_properties,
-          GetSelectorProperties(node->As<PrioritySelect>()->selector()));
-      std::string_view never_zero_str;
-      if (selector_properties.never_zero) {
-        never_zero_str = "_snz";  // selector never zero
-      }
-      return absl::StrFormat("%s_%db_%dway%s", OpToString(node->op()),
+      return absl::StrFormat("%s_%db_%dway", OpToString(node->op()),
                              node->GetType()->GetFlatBitCount(),
-                             node->operand(0)->BitCountOrDie(), never_zero_str);
+                             node->operand(0)->BitCountOrDie());
     }
     case Op::kSDiv:
     case Op::kUDiv:
@@ -2103,16 +2060,13 @@ absl::StatusOr<VerilogFunction*> ModuleBuilder::DefineFunction(Node* node) {
                                           function_name, functions_section_);
       break;
     case Op::kPrioritySel: {
-      XLS_ASSIGN_OR_RETURN(
-          SelectorProperties selector_properties,
-          GetSelectorProperties(node->As<PrioritySelect>()->selector()));
       XLS_ASSIGN_OR_RETURN(
           func,
           DefinePrioritySelectFunction(
               node->As<PrioritySelect>()->selector(), /*tpe=*/node->GetType(),
               /*num_cases=*/node->As<PrioritySelect>()->cases().size(),
               /*loc=*/node->loc(), function_name, functions_section_,
-              selector_properties, options_));
+              options_));
       break;
     }
     case Op::kUDiv:

xls/codegen/module_builder_test.cc

@@ -567,37 +567,6 @@ TEST_P(ModuleBuilderTest, TwoWayPrioritySelectAsFunction) {
                                  file.Emit());
 }
 
-TEST_P(ModuleBuilderTest, NeverZeroPrioritySelectAsFunction) {
-  VerilogFile file = NewVerilogFile();
-  Package package(TestBaseName());
-  FunctionBuilder fb(TestBaseName(), &package);
-  Type* u3 = package.GetBitsType(3);
-  Type* u32 = package.GetBitsType(32);
-  BValue s_param = fb.Param("s", u3);
-  BValue x_param = fb.Param("x", u32);
-  BValue y_param = fb.Param("y", u32);
-  BValue z_param = fb.Param("z", u32);
-  BValue d_param = fb.Param("d", u32);
-  BValue selector = fb.Or(s_param, fb.Literal(Bits::PowerOfTwo(2, 3)));
-  BValue priority_select =
-      fb.PrioritySelect(selector, {x_param, y_param, z_param}, d_param);
-  XLS_ASSERT_OK(fb.Build());
-
-  ModuleBuilder mb(TestBaseName(), &file, codegen_options());
-  XLS_ASSERT_OK_AND_ASSIGN(LogicRef * s, mb.AddInputPort("s", u3));
-  XLS_ASSERT_OK_AND_ASSIGN(LogicRef * x, mb.AddInputPort("x", u32));
-  XLS_ASSERT_OK_AND_ASSIGN(LogicRef * y, mb.AddInputPort("y", u32));
-  XLS_ASSERT_OK_AND_ASSIGN(LogicRef * z, mb.AddInputPort("z", u32));
-  XLS_ASSERT_OK_AND_ASSIGN(LogicRef * d, mb.AddInputPort("d", u32));
-
-  XLS_ASSERT_OK(mb.EmitAsAssignment("priority_select", priority_select.node(),
-                                    {s, x, y, z, d})
-                    .status());
-
-  ExpectVerilogEqualToGoldenFile(GoldenFilePath(kTestName, kTestdataPath),
-                                 file.Emit());
-}
-
 TEST_P(ModuleBuilderTest, SimilarPrioritySelects) {
   VerilogFile file = NewVerilogFile();
   Package package(TestBaseName());