From 12072a0f704796a455413f8261129807296a6605 Mon Sep 17 00:00:00 2001 From: schilkp Date: Tue, 25 Feb 2025 11:29:48 +0100 Subject: [PATCH] NOT YET MERGED: Fix zero-width FIFO materialization https://github.com/google/xls/pull/1896 --- xls/codegen/fifo_model_test_utils.h | 178 ++++++++++--- xls/codegen/materialize_fifos_pass.cc | 116 +++++---- xls/codegen/materialize_fifos_pass_test.cc | 277 +++++++++------------ xls/interpreter/block_interpreter.cc | 8 + xls/ir/instantiation.cc | 30 ++- 5 files changed, 364 insertions(+), 245 deletions(-) diff --git a/xls/codegen/fifo_model_test_utils.h b/xls/codegen/fifo_model_test_utils.h index f9e555bb8f..8d182a0c5e 100644 --- a/xls/codegen/fifo_model_test_utils.h +++ b/xls/codegen/fifo_model_test_utils.h @@ -58,102 +58,172 @@ class BaseOperation { public: virtual ~BaseOperation() = default; virtual absl::flat_hash_map InputSet() const = 0; + // Coerce operation into an operation of given the bitwidth but otherwise + // equivalent semantics. + virtual std::unique_ptr WithBitWidth(int64_t count) const = 0; }; struct Push : public BaseOperation { - explicit Push(int32_t v) : v(v) {} - int32_t v; - + explicit Push(int32_t v) : v(UBits(v, 32)) {} + explicit Push(Bits v) : v(v) {} + Bits v; absl::flat_hash_map InputSet() const override { - return { + absl::flat_hash_map result = { {std::string(FifoInstantiation::kResetPortName), Value::Bool(false)}, - {std::string(FifoInstantiation::kPushDataPortName), - Value(UBits(v, 32))}, {std::string(FifoInstantiation::kPopReadyPortName), Value::Bool(false)}, {std::string(FifoInstantiation::kPushValidPortName), Value::Bool(true)}, }; + if (v.bit_count() > 0) { + result[std::string(FifoInstantiation::kPushDataPortName)] = Value(v); + } + return result; + } + std::unique_ptr WithBitWidth(int64_t count) const override { + return std::make_unique( + Push(Bits::FromBitmap(v.bitmap().WithSize(count)))); } }; struct Pop : public BaseOperation { + explicit Pop() : bit_count(32) {}; + explicit Pop(int64_t bit_count) : bit_count(bit_count) {}; + int64_t bit_count; absl::flat_hash_map InputSet() const override { - return { + absl::flat_hash_map result = { {std::string(FifoInstantiation::kResetPortName), Value::Bool(false)}, - {std::string(FifoInstantiation::kPushDataPortName), - Value(UBits(0xf0f0f0f0, 32))}, {std::string(FifoInstantiation::kPopReadyPortName), Value::Bool(true)}, {std::string(FifoInstantiation::kPushValidPortName), Value::Bool(false)}, }; + if (bit_count > 0) { + result[std::string(FifoInstantiation::kPushDataPortName)] = + Value(UBits(0xf0f0f0f0, 32)); + } + return result; + } + std::unique_ptr WithBitWidth(int64_t count) const override { + return std::make_unique(Pop(count)); } }; struct PushAndPop : public BaseOperation { - explicit PushAndPop(int32_t v) : v(v) {} - uint32_t v; + explicit PushAndPop(int32_t v) : v(UBits(v, 32)) {} + explicit PushAndPop(Bits v) : v(v) {} + Bits v; absl::flat_hash_map InputSet() const override { - return { + absl::flat_hash_map result = { {std::string(FifoInstantiation::kResetPortName), Value::Bool(false)}, - {std::string(FifoInstantiation::kPushDataPortName), - Value(UBits(v, 32))}, {std::string(FifoInstantiation::kPopReadyPortName), Value::Bool(true)}, {std::string(FifoInstantiation::kPushValidPortName), Value::Bool(true)}, }; + if (v.bit_count() > 0) { + result[std::string(FifoInstantiation::kPushDataPortName)] = Value(v); + } + return result; + } + std::unique_ptr WithBitWidth(int64_t count) const override { + return std::make_unique( + PushAndPop(Bits::FromBitmap(v.bitmap().WithSize(count)))); } }; struct NotReady : public BaseOperation { + explicit NotReady() : bit_count(32) {}; + explicit NotReady(int64_t bit_count) : bit_count(bit_count) {}; + int64_t bit_count; absl::flat_hash_map InputSet() const override { - return { + absl::flat_hash_map result = { {std::string(FifoInstantiation::kResetPortName), Value::Bool(false)}, - {std::string(FifoInstantiation::kPushDataPortName), - Value(UBits(987654321, 32))}, {std::string(FifoInstantiation::kPopReadyPortName), Value::Bool(false)}, {std::string(FifoInstantiation::kPushValidPortName), Value::Bool(false)}, }; + if (bit_count > 0) { + result[std::string(FifoInstantiation::kPushDataPortName)] = + Value(UBits(987654321, bit_count)); + } + return result; + } + std::unique_ptr WithBitWidth(int64_t count) const override { + return std::make_unique(NotReady(count)); } }; struct ResetOp : public BaseOperation { + explicit ResetOp() : bit_count(32) {}; + explicit ResetOp(int64_t bit_count) : bit_count(bit_count) {}; + int64_t bit_count; absl::flat_hash_map InputSet() const override { - return { + absl::flat_hash_map result = { {std::string(FifoInstantiation::kResetPortName), Value::Bool(true)}, - {std::string(FifoInstantiation::kPushDataPortName), - Value(UBits(123456789, 32))}, {std::string(FifoInstantiation::kPopReadyPortName), Value::Bool(false)}, {std::string(FifoInstantiation::kPushValidPortName), Value::Bool(false)}, }; + if (bit_count > 0) { + result[std::string(FifoInstantiation::kPushDataPortName)] = + Value(UBits(123456789, bit_count)); + } + return result; + } + std::unique_ptr WithBitWidth(int64_t count) const override { + return std::make_unique(ResetOp(count)); } }; struct ResetPop : public BaseOperation { + explicit ResetPop() : bit_count(32) {}; + explicit ResetPop(int64_t bit_count) : bit_count(bit_count) {}; + int64_t bit_count; absl::flat_hash_map InputSet() const override { - return { + absl::flat_hash_map result = { {std::string(FifoInstantiation::kResetPortName), Value::Bool(true)}, - {std::string(FifoInstantiation::kPushDataPortName), - Value(UBits(123456789, 32))}, {std::string(FifoInstantiation::kPopReadyPortName), Value::Bool(true)}, {std::string(FifoInstantiation::kPushValidPortName), Value::Bool(false)}, }; + if (bit_count) { + result[std::string(FifoInstantiation::kPushDataPortName)] = + Value(UBits(123456789, bit_count)); + } + return result; + } + std::unique_ptr WithBitWidth(int64_t count) const override { + return std::make_unique(ResetPop(count)); } }; struct ResetPush : public BaseOperation { + explicit ResetPush() : bit_count(32) {}; + explicit ResetPush(int64_t bit_count) : bit_count(bit_count) {}; + int64_t bit_count; absl::flat_hash_map InputSet() const override { - return { + absl::flat_hash_map result = { {std::string(FifoInstantiation::kResetPortName), Value::Bool(true)}, - {std::string(FifoInstantiation::kPushDataPortName), - Value(UBits(123456789, 32))}, {std::string(FifoInstantiation::kPopReadyPortName), Value::Bool(false)}, {std::string(FifoInstantiation::kPushValidPortName), Value::Bool(true)}, }; + if (bit_count > 0) { + result[std::string(FifoInstantiation::kPushDataPortName)] = + Value(UBits(123456789, bit_count)); + } + return result; + } + std::unique_ptr WithBitWidth(int64_t count) const override { + return std::make_unique(ResetPush(count)); } }; struct ResetPushPop : public BaseOperation { + explicit ResetPushPop() : bit_count(32) {}; + explicit ResetPushPop(int64_t bit_count) : bit_count(bit_count) {}; + int64_t bit_count; absl::flat_hash_map InputSet() const override { - return { + absl::flat_hash_map result = { {std::string(FifoInstantiation::kResetPortName), Value::Bool(true)}, - {std::string(FifoInstantiation::kPushDataPortName), - Value(UBits(123456789, 32))}, {std::string(FifoInstantiation::kPopReadyPortName), Value::Bool(true)}, {std::string(FifoInstantiation::kPushValidPortName), Value::Bool(true)}, }; + if (bit_count > 0) { + result[std::string(FifoInstantiation::kPushDataPortName)] = + Value(UBits(123456789, bit_count)); + } + return result; + } + std::unique_ptr WithBitWidth(int64_t count) const override { + return std::make_unique(ResetPushPop(count)); } }; class Operation : public BaseOperation, @@ -165,18 +235,52 @@ class Operation : public BaseOperation, absl::flat_hash_map InputSet() const override { return std::visit([&](auto v) { return v.InputSet(); }, *this); } + std::unique_ptr WithBitWidth(int64_t count) const override { + return std::visit([&](auto v) { return v.WithBitWidth(count); }, *this); + } +}; + +struct FifoTestParam { + int64_t data_bit_count; + FifoConfig config; }; -inline auto FifoConfigDomain() { +template +void AbslStringify(Sink& sink, const FifoTestParam& value) { + absl::Format(&sink, + "kBitCount%iDepth%i%sBypass%sRegPushOutputs%sRegPopOutputs", + value.data_bit_count, value.config.depth(), + value.config.bypass() ? "" : "No", + value.config.register_push_outputs() ? "" : "No", + value.config.register_pop_outputs() ? "" : "No"); +} + +inline auto FifoTestParamDomain() { return fuzztest::Filter( - [](const FifoConfig& config) { - return !(config.depth() == 1 && config.register_pop_outputs()); + [](const FifoTestParam& params) { + if (params.config.depth() == 0 && + (!params.config.bypass() || params.config.register_push_outputs() || + params.config.register_pop_outputs())) { + // Unsupported configurations of depth=0 fifos. + return false; + } + if (params.config.depth() == 1 && + params.config.register_pop_outputs()) { + // Unsupported configuration of depth=1 fifo with + // register_pop_outputs. + return false; + } + + return true; }, - fuzztest::ConstructorOf( - /*depth=*/fuzztest::InRange(1, 10), - /*bypass=*/fuzztest::Arbitrary(), - /*register_push_outputs=*/fuzztest::Arbitrary(), - /*register_pop_outputs=*/fuzztest::Arbitrary())); + fuzztest::ConstructorOf( + /*data_bit_count=*/fuzztest::OneOf(fuzztest::Just(0), + fuzztest::Just(32)), + fuzztest::ConstructorOf( + /*depth=*/fuzztest::InRange(0, 10), + /*bypass=*/fuzztest::Arbitrary(), + /*register_push_outputs=*/fuzztest::Arbitrary(), + /*register_pop_outputs=*/fuzztest::Arbitrary()))); } inline auto OperationDomain() { diff --git a/xls/codegen/materialize_fifos_pass.cc b/xls/codegen/materialize_fifos_pass.cc index 47d7051df1..efaf53a987 100644 --- a/xls/codegen/materialize_fifos_pass.cc +++ b/xls/codegen/materialize_fifos_pass.cc @@ -52,15 +52,18 @@ absl::StatusOr MaterializeFifo(NameUniquer& uniquer, Package* p, const bool register_push = config.register_push_outputs(); const bool register_pop = config.register_pop_outputs(); Type* u1 = p->GetBitsType(1); - Type* ty = inst->data_type(); + + const bool have_data = inst->data_type()->GetFlatBitCount() > 0; // Make sure there is one extra slot at least. Bad for QOR but makes impl // easier since full is always tail + size == head - Type* buf_type = p->GetArrayType(depth + 1, ty); - Type* ptr_type = p->GetBitsType(Bits::MinBitCountUnsigned(depth + 1)); + const uint64_t slots_cnt = depth + 1; + Type* ptr_type = p->GetBitsType(Bits::MinBitCountUnsigned(slots_cnt)); + + std::string ty_name = (have_data) ? inst->data_type()->ToString() : "no_data"; BlockBuilder bb(uniquer.GetSanitizedUniqueName(absl::StrFormat( - "fifo_for_depth_%d_ty_%s_%s%s%s", depth, ty->ToString(), + "fifo_for_depth_%d_ty_%s_%s%s%s", depth, ty_name, bypass ? "with_bypass" : "no_bypass", register_pop ? "_register_pop" : "", register_push ? "_register_push" : "")), @@ -71,21 +74,13 @@ absl::StatusOr MaterializeFifo(NameUniquer& uniquer, Package* p, BValue one_lit = bb.Literal(UBits(1, ptr_type->GetFlatBitCount())); BValue depth_lit = bb.Literal(UBits(depth, ptr_type->GetFlatBitCount())); BValue long_buf_size_lit = - bb.Literal(UBits(depth + 1, ptr_type->GetFlatBitCount() + 1), + bb.Literal(UBits(slots_cnt, ptr_type->GetFlatBitCount() + 1), SourceInfo(), "long_buf_size_lit"); BValue push_valid = bb.InputPort(FifoInstantiation::kPushValidPortName, u1); BValue pop_ready_port = bb.InputPort(FifoInstantiation::kPopReadyPortName, u1); - BValue push_data = bb.InputPort(FifoInstantiation::kPushDataPortName, ty); - XLS_ASSIGN_OR_RETURN( - Register * buf_reg, - bb.block()->AddRegister( - "buf", buf_type, - xls::Reset{.reset_value = ZeroOfType(buf_type), - .asynchronous = reset_behavior.asynchronous, - .active_low = reset_behavior.active_low})); XLS_ASSIGN_OR_RETURN( Register * head_reg, bb.block()->AddRegister( @@ -108,16 +103,13 @@ absl::StatusOr MaterializeFifo(NameUniquer& uniquer, Package* p, .asynchronous = reset_behavior.asynchronous, .active_low = reset_behavior.active_low})); - BValue buf = bb.RegisterRead(buf_reg); BValue head = bb.RegisterRead(head_reg, SourceInfo(), "head_ptr"); BValue tail = bb.RegisterRead(tail_reg, SourceInfo(), "tail_ptr"); BValue slots = bb.RegisterRead(slots_reg, SourceInfo(), "slots_ptr"); // Only used if register_pop is true. std::optional pop_valid_reg; - std::optional pop_data_reg; std::optional pop_valid_reg_read; - std::optional pop_data_reg_read; std::optional pop_valid_load_en; if (register_pop) { @@ -128,22 +120,12 @@ absl::StatusOr MaterializeFifo(NameUniquer& uniquer, Package* p, xls::Reset{.reset_value = Value::Bool(false), .asynchronous = reset_behavior.asynchronous, .active_low = reset_behavior.active_low})); - XLS_ASSIGN_OR_RETURN( - pop_data_reg, - bb.block()->AddRegister( - "pop_data_reg", ty, /*reset=*/ - xls::Reset{.reset_value = ZeroOfType(ty), - .asynchronous = reset_behavior.asynchronous, - .active_low = reset_behavior.active_low})); pop_valid_reg_read = bb.RegisterRead(*pop_valid_reg); depth_lit = bb.Add(depth_lit, bb.ZeroExtend(*pop_valid_reg_read, ptr_type->GetFlatBitCount())); - pop_data_reg_read = bb.RegisterRead(*pop_data_reg); pop_valid_load_en = bb.Or(pop_ready_port, bb.Not(*pop_valid_reg_read)); } - BValue current_queue_tail = bb.ArrayIndex(buf, {tail}); - auto add_mod_buf_size = [&](BValue val, BValue addend, std::optional name) -> BValue { // NB Need to be sure to not run into issues where the 2 mods here (explicit @@ -189,10 +171,12 @@ absl::StatusOr MaterializeFifo(NameUniquer& uniquer, Package* p, BValue push_ready; BValue pop_valid; - BValue next_buf_value_if_push_occurs = bb.ArrayUpdate(buf, push_data, {head}); - BValue pop_data_value; BValue did_pop_occur_bool; BValue did_push_occur_bool; + + // Only used if bypass is true. + std::optional is_empty_bool = std::nullopt; + if (bypass) { // NB No need to handle the 'full and bypass and both read & write case // specially since we have an extra slot to store those values in. @@ -201,14 +185,13 @@ absl::StatusOr MaterializeFifo(NameUniquer& uniquer, Package* p, BValue is_popable = bb.Or(buf_not_empty_bool, bypass_possible); pop_valid = bb.Or(buf_not_empty_bool, push_valid); push_ready = bb.Or(can_do_push, buf_pop_ready); - BValue is_empty_bool = bb.Eq(head, tail); - BValue did_no_write_bypass_occur = bb.And(is_empty_bool, bypass_possible); + is_empty_bool = bb.Eq(head, tail); + BValue did_no_write_bypass_occur = bb.And(*is_empty_bool, bypass_possible); BValue did_visible_push_occur_bool = bb.And(is_pushable, push_valid); if (register_pop) { buf_pop_ready = bb.And(buf_pop_ready, pop_valid); } BValue did_visible_pop_occur_bool = bb.And(is_popable, buf_pop_ready); - pop_data_value = bb.Select(is_empty_bool, {current_queue_tail, push_data}); did_pop_occur_bool = bb.And(did_visible_pop_occur_bool, bb.Not(did_no_write_bypass_occur), SourceInfo(), "did_pop_occur"); @@ -217,7 +200,6 @@ absl::StatusOr MaterializeFifo(NameUniquer& uniquer, Package* p, } else { push_ready = can_do_push; pop_valid = buf_not_empty_bool; - pop_data_value = current_queue_tail; did_pop_occur_bool = bb.And(pop_valid, buf_pop_ready, SourceInfo(), "did_pop_occur"); did_push_occur_bool = bb.And(push_ready, push_valid); @@ -230,30 +212,20 @@ absl::StatusOr MaterializeFifo(NameUniquer& uniquer, Package* p, if (register_pop) { bb.RegisterWrite(*pop_valid_reg, pop_valid, /*load_enable=*/pop_valid_load_en, reset_port); - bb.RegisterWrite(*pop_data_reg, pop_data_value, - /*load_enable=*/pop_valid_load_en, reset_port); pop_valid = *pop_valid_reg_read; - pop_data_value = *pop_data_reg_read; } bb.OutputPort(FifoInstantiation::kPushReadyPortName, push_ready); // Empty is head == tail so ne means there's something to pop. bb.OutputPort(FifoInstantiation::kPopValidPortName, pop_valid); - // We could always send the current data. If not r/v the data is ignored - // anyway. Since this is testing might as well send mux in a zero if we - // aren't ready. - bb.OutputPort(FifoInstantiation::kPopDataPortName, pop_data_value); - BValue pushed = bb.And(push_ready, push_valid, SourceInfo(), "pushed"); BValue popped = bb.And(pop_ready_port, pop_valid, SourceInfo(), "popped"); BValue slots_next = bb.Select( pushed, {bb.Select(popped, {slots, bb.Subtract(slots, one_lit)}), bb.Select(popped, {bb.Add(slots, one_lit), slots})}); - bb.RegisterWrite(buf_reg, next_buf_value_if_push_occurs, - /*load_enable=*/did_push_occur_bool, reset_port); bb.RegisterWrite(head_reg, next_head_value_if_push_occurs, /*load_enable=*/did_push_occur_bool, reset_port); bb.RegisterWrite(tail_reg, next_tail_value_if_pop_occurs, @@ -261,6 +233,66 @@ absl::StatusOr MaterializeFifo(NameUniquer& uniquer, Package* p, bb.RegisterWrite(slots_reg, slots_next, /*load_enable=*/std::nullopt, reset_port); + // Only generate data path and I/O if the data type is of non-zero wdith. + if (have_data) { + Type* ty = inst->data_type(); + Type* buf_type = p->GetArrayType(slots_cnt, ty); + + BValue push_data = bb.InputPort(FifoInstantiation::kPushDataPortName, ty); + + XLS_ASSIGN_OR_RETURN( + Register * buf_reg, + bb.block()->AddRegister( + "buf", buf_type, + xls::Reset{.reset_value = ZeroOfType(buf_type), + .asynchronous = reset_behavior.asynchronous, + .active_low = reset_behavior.active_low})); + + BValue buf = bb.RegisterRead(buf_reg); + + // Only used if register_pop is true. + std::optional pop_data_reg; + std::optional pop_data_reg_read; + + if (register_pop) { + XLS_ASSIGN_OR_RETURN( + pop_data_reg, + bb.block()->AddRegister( + "pop_data_reg", ty, /*reset=*/ + xls::Reset{.reset_value = ZeroOfType(ty), + .asynchronous = reset_behavior.asynchronous, + .active_low = reset_behavior.active_low})); + pop_data_reg_read = bb.RegisterRead(*pop_data_reg); + } + + BValue current_queue_tail = bb.ArrayIndex(buf, {tail}); + + BValue next_buf_value_if_push_occurs = + bb.ArrayUpdate(buf, push_data, {head}); + + BValue pop_data_value; + if (bypass) { + pop_data_value = + bb.Select(*is_empty_bool, {current_queue_tail, push_data}); + } else { + pop_data_value = current_queue_tail; + } + + if (register_pop) { + bb.RegisterWrite(*pop_data_reg, pop_data_value, + /*load_enable=*/pop_valid_load_en, reset_port); + pop_data_value = *pop_data_reg_read; + } + + // We could always send the current data. If not r/v the data is ignored + // anyway. Since this is testing might as well send mux in a zero if we + // aren't ready. + bb.OutputPort(FifoInstantiation::kPopDataPortName, pop_data_value); + + bb.RegisterWrite(buf_reg, next_buf_value_if_push_occurs, + /*load_enable=*/did_push_occur_bool, reset_port); + } + return bb.Build(); } } // namespace diff --git a/xls/codegen/materialize_fifos_pass_test.cc b/xls/codegen/materialize_fifos_pass_test.cc index 54e6963b5f..e9d59d0353 100644 --- a/xls/codegen/materialize_fifos_pass_test.cc +++ b/xls/codegen/materialize_fifos_pass_test.cc @@ -60,17 +60,14 @@ class MaterializeFifosPassTestHelper { kInterpreterBlockEvaluator; static constexpr const BlockEvaluator& kTestEvaluator = kInterpreterBlockEvaluator; - absl::StatusOr MakeOracleBlock(Package* p, FifoConfig config) { - return MakeOracleBlock(p, config, p->GetBitsType(32)); - } - absl::StatusOr MakeOracleBlock(Package* p, FifoConfig config, - Type* ty, + absl::StatusOr MakeOracleBlock(Package* p, FifoTestParam params, std::string_view name = "oracle") { BlockBuilder bb(uniq_.GetSanitizedUniqueName( absl::StrFormat("%s_%s", name, TestName())), p); - XLS_ASSIGN_OR_RETURN(auto inst, - bb.block()->AddFifoInstantiation("fifo", config, ty)); + XLS_ASSIGN_OR_RETURN(auto inst, bb.block()->AddFifoInstantiation( + "fifo", params.config, + p->GetBitsType(params.data_bit_count))); XLS_ASSIGN_OR_RETURN(InstantiationType inst_ty, inst->type()); for (const auto& [port_name, in_ty] : inst_ty.input_types()) { bb.InstantiationInput(inst, port_name, bb.InputPort(port_name, in_ty)); @@ -80,13 +77,8 @@ class MaterializeFifosPassTestHelper { } return bb.Build(); } - absl::StatusOr MakeFifoBlock(Package* p, FifoConfig config) { - return MakeFifoBlock(p, config, p->GetBitsType(32)); - } - absl::StatusOr MakeFifoBlock(Package* p, FifoConfig config, - Type* ty) { - XLS_ASSIGN_OR_RETURN(Block * wrapper, - MakeOracleBlock(p, config, ty, "test")); + absl::StatusOr MakeFifoBlock(Package* p, FifoTestParam params) { + XLS_ASSIGN_OR_RETURN(Block * wrapper, MakeOracleBlock(p, params, "test")); MaterializeFifosPass mfp; CodegenPassUnit pu(p, wrapper); @@ -116,7 +108,7 @@ class MaterializeFifosPassTestHelper { } // Compare against the interpreter as an oracle. - void RunTestVector(FifoConfig config, + void RunTestVector(FifoTestParam config, absl::Span operations) { auto p = CreatePackage(); XLS_ASSERT_OK_AND_ASSIGN(Block * fifo_block, @@ -129,22 +121,26 @@ class MaterializeFifosPassTestHelper { MakeOracle(oracle_package.get(), config)); testing::Test::RecordProperty("oracle", oracle_package->DumpIr()); int64_t i = 0; - for (const BaseOperation& op : operations) { + for (const BaseOperation& orig_op : operations) { + // Ensure applied operation has the expected type: + std::unique_ptr op = + orig_op.WithBitWidth(config.data_bit_count); + ScopedMaybeRecord op_cnt("op_cnt", i); ScopedMaybeRecord init_state("initial_state", tester->registers()); - auto test_res = tester->RunOneCycle(op.InputSet()); - auto oracle_res = oracle->RunOneCycle(op.InputSet()); + auto test_res = tester->RunOneCycle(op->InputSet()); + auto oracle_res = oracle->RunOneCycle(op->InputSet()); ASSERT_THAT(std::make_pair(test_res, oracle_res), testing::Pair(absl_testing::IsOk(), absl_testing::IsOk())) << "@i=" << i; auto tester_outputs = tester->output_ports(); ScopedMaybeRecord state("out_state", tester->registers()); - ScopedMaybeRecord input("input", op.InputSet()); + ScopedMaybeRecord input("input", op->InputSet()); ScopedMaybeRecord test_out("test_output", tester_outputs); ScopedMaybeRecord oracle_out("oracle_output", oracle->output_ports()); ScopedMaybeRecord trace("trace", tester->events().trace_msgs); ASSERT_THAT(tester_outputs, - UsableOutputsMatch(op.InputSet(), oracle->output_ports())) + UsableOutputsMatch(op->InputSet(), oracle->output_ports())) << "@" << i << ". tester_state: {" << absl::StrJoin(tester->registers(), ", ", absl::PairFormatter("=")) << "}"; @@ -154,26 +150,11 @@ class MaterializeFifosPassTestHelper { NameUniquer uniq_ = NameUniquer("___"); }; -struct PartialConfig { - bool bypass; - bool reg_push_outputs; - bool reg_pop_outputs; - - FifoConfig WithDepth(int64_t depth) const { - return FifoConfig(depth, bypass, reg_push_outputs, reg_pop_outputs); - } -}; -template -void AbslStringify(Sink& sink, const PartialConfig& value) { - absl::Format(&sink, "k%sBypass%sRegPushOutputs%sRegPopOutputs", - value.bypass ? "" : "No", value.reg_push_outputs ? "" : "No", - value.reg_pop_outputs ? "" : "No"); -} class MaterializeFifosPassTest : public MaterializeFifosPassTestHelper, public IrTestBase, - public testing::WithParamInterface { + public testing::WithParamInterface { public: std::string TestName() const override { return IrTestBase::TestName(); } std::unique_ptr CreatePackage() const override { @@ -182,74 +163,74 @@ class MaterializeFifosPassTest }; TEST_P(MaterializeFifosPassTest, BasicFifo) { - RunTestVector(GetParam().WithDepth(3), { - Push{1}, - Push{2}, - Push{3}, - NotReady{}, - NotReady{}, - NotReady{}, - Pop{}, - Pop{}, - Pop{}, - // Out of elements. - Pop{}, - Pop{}, - PushAndPop{4}, - PushAndPop{5}, - PushAndPop{6}, - PushAndPop{7}, - Pop{}, - // Out of elements - Pop{}, - Pop{}, - Push{8}, - Push{9}, - Push{10}, - // Full - Push{11}, - Push{12}, - Push{13}, - Pop{}, - Pop{}, - Pop{}, - // Out of elements. - Pop{}, - Pop{}, - }); + RunTestVector(GetParam(), { + Push{1}, + Push{2}, + Push{3}, + NotReady{}, + NotReady{}, + NotReady{}, + Pop{}, + Pop{}, + Pop{}, + // Out of elements. + Pop{}, + Pop{}, + PushAndPop{4}, + PushAndPop{5}, + PushAndPop{6}, + PushAndPop{7}, + Pop{}, + // Out of elements + Pop{}, + Pop{}, + Push{8}, + Push{9}, + Push{10}, + // Full + Push{11}, + Push{12}, + Push{13}, + Pop{}, + Pop{}, + Pop{}, + // Out of elements. + Pop{}, + Pop{}, + }); } TEST_P(MaterializeFifosPassTest, BasicFifoFull) { - RunTestVector(GetParam().WithDepth(3), { - Push{1}, - Push{2}, - Push{3}, - Push{4}, - Push{5}, - Push{6}, - Push{7}, - PushAndPop{8}, - PushAndPop{9}, - PushAndPop{10}, - PushAndPop{11}, - Pop{}, - Pop{}, - Pop{}, - Pop{}, - Pop{}, - Pop{}, - }); + RunTestVector(GetParam(), { + Push{1}, + Push{2}, + Push{3}, + Push{4}, + Push{5}, + Push{6}, + Push{7}, + PushAndPop{8}, + PushAndPop{9}, + PushAndPop{10}, + PushAndPop{11}, + Pop{}, + Pop{}, + Pop{}, + Pop{}, + Pop{}, + Pop{}, + }); } TEST_P(MaterializeFifosPassTest, FifoLengthIsSmall) { // If length is near the overflow point for the bit-width mistakes in mod // calculation are possible. - RunTestVector(GetParam().WithDepth(2), + RunTestVector(GetParam(), {PushAndPop{1}, PushAndPop{2}, PushAndPop{3}, PushAndPop{4}}); } TEST_F(MaterializeFifosPassTest, ResetFullFifo) { - FifoConfig cfg(3, false, false, false); + FifoTestParam cfg{32, {3, false, false, false}}; RunTestVector(cfg, { Push{1}, Push{2}, @@ -273,67 +254,55 @@ TEST_F(MaterializeFifosPassTest, ResetFullFifo) { } TEST_P(MaterializeFifosPassTest, BasicFifoBypass) { - RunTestVector(GetParam().WithDepth(3), { - PushAndPop{1}, - PushAndPop{2}, - PushAndPop{3}, - PushAndPop{4}, - PushAndPop{5}, - PushAndPop{6}, - PushAndPop{7}, - PushAndPop{8}, - Pop{}, - Pop{}, - Pop{}, - Pop{}, - Pop{}, - Pop{}, - }); + RunTestVector(GetParam(), { + PushAndPop{1}, + PushAndPop{2}, + PushAndPop{3}, + PushAndPop{4}, + PushAndPop{5}, + PushAndPop{6}, + PushAndPop{7}, + PushAndPop{8}, + Pop{}, + Pop{}, + Pop{}, + Pop{}, + Pop{}, + Pop{}, + }); +} + +inline std::vector GenerateMaterializeFifoTestParams() { + std::vector params; + for (int64_t data_bit_count : {0, 32}) { + for (int64_t depth : {0, 3, 5}) { + for (bool bypass : {true, false}) { + for (bool register_push_outputs : {true, false}) { + for (bool register_pop_outputs : {true, false}) { + if (depth == 0 && + (!bypass || register_push_outputs || register_pop_outputs)) { + // Unsupported configurations of depth=0 fifos. + continue; + } + if (depth == 1 && register_pop_outputs) { + // Unsupported configuration of depth=1 fifo with + // register_pop_outputs. + continue; + } + params.push_back(FifoTestParam{ + .data_bit_count = data_bit_count, + .config = FifoConfig(depth, bypass, register_push_outputs, + register_pop_outputs)}); + } + } + } + } + } + return params; } INSTANTIATE_TEST_SUITE_P(MaterializeFifosPassTest, MaterializeFifosPassTest, - testing::ValuesIn({ - PartialConfig{ - .bypass = false, - .reg_push_outputs = false, - .reg_pop_outputs = false, - }, - PartialConfig{ - .bypass = false, - .reg_push_outputs = true, - .reg_pop_outputs = false, - }, - PartialConfig{ - .bypass = false, - .reg_push_outputs = false, - .reg_pop_outputs = true, - }, - PartialConfig{ - .bypass = false, - .reg_push_outputs = true, - .reg_pop_outputs = true, - }, - PartialConfig{ - .bypass = true, - .reg_push_outputs = false, - .reg_pop_outputs = false, - }, - PartialConfig{ - .bypass = true, - .reg_push_outputs = true, - .reg_pop_outputs = false, - }, - PartialConfig{ - .bypass = true, - .reg_push_outputs = false, - .reg_pop_outputs = true, - }, - PartialConfig{ - .bypass = true, - .reg_push_outputs = true, - .reg_pop_outputs = true, - }, - }), + testing::ValuesIn(GenerateMaterializeFifoTestParams()), testing::PrintToStringParamName()); class MaterializeFifosPassFuzzTest : public MaterializeFifosPassTestHelper { @@ -343,22 +312,22 @@ class MaterializeFifosPassFuzzTest : public MaterializeFifosPassTestHelper { } std::string TestName() const override { return "FuzzTest"; } }; -void FuzzTestFifo(FifoConfig cfg, const std::vector& ops) { +void FuzzTestFifo(FifoTestParam cfg, const std::vector& ops) { MaterializeFifosPassFuzzTest fixture; fixture.RunTestVector(cfg, ops); } FUZZ_TEST(MaterializeFifosPassFuzzTest, FuzzTestFifo) - .WithDomains(FifoConfigDomain(), + .WithDomains(FifoTestParamDomain(), fuzztest::VectorOf(OperationDomain()).WithMaxSize(1000)); TEST(FifoFuzzTest, FuzzTestJitRegression) { - FuzzTestFifo(xls::FifoConfig(1, false, false, false), + FuzzTestFifo(FifoTestParam{32, xls::FifoConfig(1, false, false, false)}, {Operation(NotReady()), Operation(Pop())}); } TEST(FifoFuzzTest, FuzzTestJitRegression2) { - FuzzTestFifo(xls::FifoConfig(10, true, false, false), + FuzzTestFifo(FifoTestParam{32, xls::FifoConfig(10, true, false, false)}, {Operation(ResetOp()), Operation(ResetOp())}); } diff --git a/xls/interpreter/block_interpreter.cc b/xls/interpreter/block_interpreter.cc index 0689d05434..a31767b642 100644 --- a/xls/interpreter/block_interpreter.cc +++ b/xls/interpreter/block_interpreter.cc @@ -185,6 +185,14 @@ class FifoModel { return absl::InvalidArgumentError( absl::StrFormat("Unexpected port '%s'", input->port_name())); } + + // If this fifo does not actually carry any data, we don't require + // the `push_data_` input to have been provided. The presence/absence + // of `push_valid_` is all that is required. + if (type_->GetFlatBitCount() == 0) { + push_data_ = ZeroOfType(type_); + } + if (!(push_valid_.has_value() && push_data_.has_value() && pop_ready_.has_value() && reset_.has_value())) { // Not yet ready to compute next state. diff --git a/xls/ir/instantiation.cc b/xls/ir/instantiation.cc index e784493c98..f7d078fc65 100644 --- a/xls/ir/instantiation.cc +++ b/xls/ir/instantiation.cc @@ -282,18 +282,24 @@ absl::StatusOr FifoInstantiation::GetInputPort( absl::StatusOr FifoInstantiation::type() const { Type* u1 = package_->GetBitsType(1); - return InstantiationType(/*input_types=*/ - { - {std::string{kResetPortName}, u1}, - {std::string(kPushValidPortName), u1}, - {std::string(kPopReadyPortName), u1}, - {std::string(kPushDataPortName), data_type()}, - }, - /*output_types=*/{ - {std::string(kPopValidPortName), u1}, - {std::string(kPushReadyPortName), u1}, - {std::string(kPopDataPortName), data_type()}, - }); + + absl::flat_hash_map input_types = { + {std::string{kResetPortName}, u1}, + {std::string(kPushValidPortName), u1}, + {std::string(kPopReadyPortName), u1}, + }; + + absl::flat_hash_map output_types = { + {std::string(kPopValidPortName), u1}, + {std::string(kPushReadyPortName), u1}, + }; + + if (data_type()->GetFlatBitCount() > 0) { + input_types[std::string(kPushDataPortName)] = data_type(); + output_types[std::string(kPopDataPortName)] = data_type(); + } + + return InstantiationType(input_types, output_types); } absl::StatusOr FifoInstantiation::GetOutputPort(