Skip to content

Commit 5cb393f

Browse files
grebecopybara-github
grebe
authored and
copybara-github
committed
Add more options to FIFO parameterization.
The `bypass` parameter in `FifoConfig` is not sufficient to eliminate combinational paths between push- and pop-side signals. This change adds `register_push_outputs` and `register_pop_outputs`, parameters that control if the push- or pop-side outputs are registered (breaking combinational paths). Also, the config is getting big enough that I changed it from a struct (where some values are often unspecified) to a class with a constructor that requires all members to be specified. It seems better to be explicit everywhere, even if it's annoying (especially because there are so many `bool`s, which should be disambiguated with `/*param_name=*/`-style comments. Ideally, I think it will be best to remove FIFO-config from channels and bind FIFO-config to channels at codegen time. See also #1391 for more discussion. PiperOrigin-RevId: 631217053
1 parent 7d497f2 commit 5cb393f

27 files changed

+527
-315
lines changed

Diff for: ‎xls/codegen/block_generator.cc

+30-10
Original file line numberDiff line numberDiff line change
@@ -794,30 +794,50 @@ class BlockGenerator {
794794
dynamic_cast<FifoInstantiation*>(instantiation)) {
795795
std::initializer_list<Connection> parameters{
796796
Connection{
797-
"Width",
798-
mb_.file()->Literal(
797+
.port_name = "Width",
798+
.expression = mb_.file()->Literal(
799799
UBits(fifo_instantiation->data_type()->GetFlatBitCount(),
800800
32),
801801
SourceInfo(),
802802
/*format=*/FormatPreference::kUnsignedDecimal)},
803-
Connection{"Depth",
804-
mb_.file()->Literal(
805-
UBits(fifo_instantiation->fifo_config().depth, 32),
803+
Connection{.port_name = "Depth",
804+
.expression = mb_.file()->Literal(
805+
UBits(fifo_instantiation->fifo_config().depth(), 32),
806806
SourceInfo(),
807807
/*format=*/FormatPreference::kUnsignedDecimal)},
808808
Connection{
809-
"EnableBypass",
810-
mb_.file()->Literal(
811-
UBits(fifo_instantiation->fifo_config().bypass ? 1 : 0, 1),
809+
.port_name = "EnableBypass",
810+
.expression = mb_.file()->Literal(
811+
UBits(fifo_instantiation->fifo_config().bypass() ? 1 : 0,
812+
1),
813+
SourceInfo(),
814+
/*format=*/FormatPreference::kUnsignedDecimal)},
815+
Connection{.port_name = "RegisterPushOutputs",
816+
.expression = mb_.file()->Literal(
817+
UBits(fifo_instantiation->fifo_config()
818+
.register_push_outputs()
819+
? 1
820+
: 0,
821+
1),
822+
SourceInfo(),
823+
/*format=*/FormatPreference::kUnsignedDecimal)},
824+
Connection{
825+
.port_name = "RegisterPopOutputs",
826+
.expression = mb_.file()->Literal(
827+
UBits(
828+
fifo_instantiation->fifo_config().register_pop_outputs()
829+
? 1
830+
: 0,
831+
1),
812832
SourceInfo(),
813833
/*format=*/FormatPreference::kUnsignedDecimal)},
814834
};
815835

816836
// Append clock and reset to the front of connections.
817837
XLS_RET_CHECK(mb_.reset().has_value());
818838
std::vector<Connection> appended_connections{
819-
Connection{"clk", mb_.clock()},
820-
Connection{"rst", mb_.reset()->signal},
839+
Connection{.port_name = "clk", .expression = mb_.clock()},
840+
Connection{.port_name = "rst", .expression = mb_.reset()->signal},
821841
};
822842
appended_connections.reserve(connections.size() + 2);
823843
std::move(connections.begin(), connections.end(),

Diff for: ‎xls/codegen/block_generator_test.cc

+5-2
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,9 @@ push_ready, push_data, push_valid,
8989
pop_ready, pop_data, pop_valid);
9090
parameter Width = 32,
9191
Depth = 32,
92-
EnableBypass = 0;
92+
EnableBypass = 0,
93+
RegisterPushOutputs = 0,
94+
RegisterPopOutputs = 0;
9395
localparam AddrWidth = $clog2(Depth) + 1;
9496
input wire clk;
9597
input wire rst;
@@ -102,7 +104,8 @@ pop_ready, pop_data, pop_valid);
102104
103105
// Require depth be 1 and bypass disabled.
104106
initial begin
105-
if (EnableBypass || Depth != 1) begin
107+
if (EnableBypass || Depth != 1 || RegisterPushOutputs ||
108+
RegisterPopOutputs) begin
106109
$fatal("FIFO configuration not supported.");
107110
end
108111
end

Diff for: ‎xls/codegen/module_signature_test.cc

+40-27
Original file line numberDiff line numberDiff line change
@@ -179,8 +179,10 @@ TEST(ModuleSignatureTest, StreamingChannelsInterface) {
179179
b.AddStreamingChannel(
180180
"streaming_in", ChannelOps::kReceiveOnly, FlowControl::kReadyValid,
181181
p.GetTupleType({p.GetBitsType(32)}),
182-
/*fifo_config=*/FifoConfig{.depth = 42}, "streaming_in_data",
183-
"streaming_in_valid", "streaming_in_ready");
182+
/*fifo_config=*/
183+
FifoConfig(/*depth=*/42, /*bypass=*/true, /*register_push_outputs=*/true,
184+
/*register_pop_outputs=*/false),
185+
"streaming_in_data", "streaming_in_valid", "streaming_in_ready");
184186

185187
b.AddStreamingChannel("streaming_out", ChannelOps::kSendOnly,
186188
FlowControl::kNone, p.GetBitsType(32),
@@ -233,11 +235,13 @@ TEST(ModuleSignatureTest, GetByName) {
233235
b.AddDataInputAsBits("streaming_in_valid", 1);
234236
b.AddDataOutputAsBits("streaming_in_ready", 1);
235237

236-
b.AddStreamingChannel("streaming_in", ChannelOps::kReceiveOnly,
237-
FlowControl::kReadyValid, p.GetBitsType(24),
238-
/*fifo_config=*/FifoConfig{.depth = 42},
239-
"streaming_in_data", "streaming_in_valid",
240-
"streaming_in_ready");
238+
b.AddStreamingChannel(
239+
"streaming_in", ChannelOps::kReceiveOnly, FlowControl::kReadyValid,
240+
p.GetBitsType(24),
241+
/*fifo_config=*/
242+
FifoConfig(/*depth=*/42, /*bypass=*/true, /*register_push_outputs=*/true,
243+
/*register_pop_outputs=*/false),
244+
"streaming_in_data", "streaming_in_valid", "streaming_in_ready");
241245

242246
b.AddDataOutputAsBits("single_val_out_port", 64);
243247

@@ -287,11 +291,13 @@ TEST(ModuleSignatureTest, GetChannels) {
287291
b.AddDataInputAsBits("streaming_in_valid", 1);
288292
b.AddDataOutputAsBits("streaming_in_ready", 1);
289293

290-
b.AddStreamingChannel("streaming_in", ChannelOps::kReceiveOnly,
291-
FlowControl::kReadyValid, p.GetBitsType(24),
292-
/*fifo_config=*/FifoConfig{.depth = 42},
293-
"streaming_in_data", "streaming_in_valid",
294-
"streaming_in_ready");
294+
b.AddStreamingChannel(
295+
"streaming_in", ChannelOps::kReceiveOnly, FlowControl::kReadyValid,
296+
p.GetBitsType(24),
297+
/*fifo_config=*/
298+
FifoConfig(/*depth=*/42, /*bypass=*/true, /*register_push_outputs=*/true,
299+
/*register_pop_outputs=*/false),
300+
"streaming_in_data", "streaming_in_valid", "streaming_in_ready");
295301

296302
b.AddDataOutputAsBits("single_val_out_port", 64);
297303

@@ -319,11 +325,13 @@ TEST(ModuleSignatureTest, GetChannelNameWith) {
319325
b.AddDataInputAsBits("streaming_in_valid", 1);
320326
b.AddDataOutputAsBits("streaming_in_ready", 1);
321327

322-
b.AddStreamingChannel("streaming_in", ChannelOps::kReceiveOnly,
323-
FlowControl::kReadyValid, p.GetBitsType(24),
324-
/*fifo_config=*/FifoConfig{.depth = 42},
325-
"streaming_in_data", "streaming_in_valid",
326-
"streaming_in_ready");
328+
b.AddStreamingChannel(
329+
"streaming_in", ChannelOps::kReceiveOnly, FlowControl::kReadyValid,
330+
p.GetBitsType(24),
331+
/*fifo_config=*/
332+
FifoConfig(/*depth=*/42, /*bypass=*/true, /*register_push_outputs=*/true,
333+
/*register_pop_outputs=*/false),
334+
"streaming_in_data", "streaming_in_valid", "streaming_in_ready");
327335

328336
b.AddDataOutputAsBits("single_val_out_port", 64);
329337

@@ -358,11 +366,13 @@ TEST(ModuleSignatureTest, RemoveStreamingChannel) {
358366

359367
b.AddDataOutputAsBits("streaming_out_data", 16);
360368

361-
b.AddStreamingChannel("streaming_in", ChannelOps::kReceiveOnly,
362-
FlowControl::kReadyValid, p.GetBitsType(24),
363-
/*fifo_config=*/FifoConfig{.depth = 42},
364-
"streaming_in_data", "streaming_in_valid",
365-
"streaming_in_ready");
369+
b.AddStreamingChannel(
370+
"streaming_in", ChannelOps::kReceiveOnly, FlowControl::kReadyValid,
371+
p.GetBitsType(24),
372+
/*fifo_config=*/
373+
FifoConfig(/*depth=*/42, /*bypass=*/true, /*register_push_outputs=*/true,
374+
/*register_pop_outputs=*/false),
375+
"streaming_in_data", "streaming_in_valid", "streaming_in_ready");
366376

367377
b.AddStreamingChannel("streaming_out", ChannelOps::kSendOnly,
368378
FlowControl::kNone, p.GetBitsType(24),
@@ -568,11 +578,14 @@ TEST(ModuleSignatureTest, FifoInstantiation) {
568578

569579
XLS_ASSERT_OK_AND_ASSIGN(
570580
StreamingChannel * loopback_channel,
571-
p.CreateStreamingChannel(
572-
"loopback_channel", ChannelOps::kSendReceive,
573-
p.GetTupleType({p.GetBitsType(32)}), /*initial_values=*/{},
574-
/*fifo_config=*/FifoConfig{.depth = 10, .bypass = false},
575-
/*flow_control=*/FlowControl::kReadyValid));
581+
p.CreateStreamingChannel("loopback_channel", ChannelOps::kSendReceive,
582+
p.GetTupleType({p.GetBitsType(32)}),
583+
/*initial_values=*/{},
584+
/*fifo_config=*/
585+
FifoConfig(/*depth=*/10, /*bypass=*/false,
586+
/*register_push_outputs=*/true,
587+
/*register_pop_outputs=*/false),
588+
/*flow_control=*/FlowControl::kReadyValid));
576589
ModuleSignatureBuilder b(TestName());
577590

578591
// Add ports for streaming channels.

Diff for: ‎xls/codegen/signature_generator_test.cc

+4-4
Original file line numberDiff line numberDiff line change
@@ -354,11 +354,11 @@ TEST(SignatureGeneratorTest, IOSignatureProcToPipelinedBLock) {
354354
}
355355

356356
TEST(SignatureGeneratorTest, BlockWithFifoInstantiationNoChannel) {
357-
constexpr std::string_view ir_text =R"(package test
357+
constexpr std::string_view ir_text = R"(package test
358358
359359
block my_block(in: bits[32], out: (bits[32])) {
360360
in: bits[32] = input_port(name=in)
361-
instantiation my_inst(data_type=(bits[32]), depth=3, bypass=false, kind=fifo)
361+
instantiation my_inst(data_type=(bits[32]), depth=3, bypass=false, register_push_outputs=false, register_pop_outputs=false, kind=fifo)
362362
in_inst_input: () = instantiation_input(in, instantiation=my_inst, port_name=push_data)
363363
pop_data_inst_output: (bits[32]) = instantiation_output(instantiation=my_inst, port_name=pop_data)
364364
out_output_port: () = output_port(pop_data_inst_output, name=out)
@@ -393,7 +393,7 @@ block my_block(in: bits[32], out: (bits[32])) {
393393

394394
TEST(SignatureGeneratorTest, BlockWithFifoInstantiationWithChannel) {
395395
constexpr std::string_view ir_text = R"(package test
396-
chan a(bits[32], id=0, ops=send_only, fifo_depth=3, bypass=false, kind=streaming, flow_control=ready_valid, metadata="")
396+
chan a(bits[32], id=0, ops=send_only, fifo_depth=3, bypass=false, register_push_outputs=false, register_pop_outputs=false, kind=streaming, flow_control=ready_valid, metadata="")
397397
398398
proc needed_to_verify(tok: token, state: (), init={()}) {
399399
literal0: bits[32] = literal(value=32)
@@ -403,7 +403,7 @@ proc needed_to_verify(tok: token, state: (), init={()}) {
403403
404404
block my_block(in: bits[32], out: (bits[32])) {
405405
in: bits[32] = input_port(name=in)
406-
instantiation my_inst(data_type=(bits[32]), depth=3, bypass=false, channel=a, kind=fifo)
406+
instantiation my_inst(data_type=(bits[32]), depth=3, bypass=false, register_push_outputs=false, register_pop_outputs=false, channel=a, kind=fifo)
407407
in_inst_input: () = instantiation_input(in, instantiation=my_inst, port_name=push_data)
408408
pop_data_inst_output: (bits[32]) = instantiation_output(instantiation=my_inst, port_name=pop_data)
409409
out_output_port: () = output_port(pop_data_inst_output, name=out)

Diff for: ‎xls/codegen/testdata/block_generator_test_LoopbackFifoInstantiation.svtxt

+3-1
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,9 @@ module running_sum(
128128
xls_fifo_wrapper #(
129129
.Width(32'd32),
130130
.Depth(32'd1),
131-
.EnableBypass(1'd0)
131+
.EnableBypass(1'd0),
132+
.RegisterPushOutputs(1'd0),
133+
.RegisterPopOutputs(1'd0)
132134
) fifo_loopback (
133135
.clk(clk),
134136
.rst(rst),

Diff for: ‎xls/codegen/testdata/block_generator_test_LoopbackFifoInstantiation.vtxt

+3-1
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,9 @@ module running_sum(
128128
xls_fifo_wrapper #(
129129
.Width(32'd32),
130130
.Depth(32'd1),
131-
.EnableBypass(1'd0)
131+
.EnableBypass(1'd0),
132+
.RegisterPushOutputs(1'd0),
133+
.RegisterPopOutputs(1'd0)
132134
) fifo_loopback (
133135
.clk(clk),
134136
.rst(rst),

Diff for: ‎xls/contrib/xlscc/translate_loops.cc

+8-2
Original file line numberDiff line numberDiff line change
@@ -596,7 +596,10 @@ absl::Status Translator::GenerateIR_PipelinedLoop(
596596
package_->CreateStreamingChannel(
597597
ch_name, xls::ChannelOps::kSendReceive, context_out_xls_type,
598598
/*initial_values=*/{},
599-
/*fifo_config=*/xls::FifoConfig{.depth = 0},
599+
/*fifo_config=*/
600+
xls::FifoConfig(/*depth=*/0, /*bypass=*/true,
601+
/*register_push_outputs=*/false,
602+
/*register_pop_outputs=*/false),
600603
xls::FlowControl::kReadyValid));
601604
}
602605
IOChannel new_channel;
@@ -615,7 +618,10 @@ absl::Status Translator::GenerateIR_PipelinedLoop(
615618
ch_name, xls::ChannelOps::kSendReceive,
616619
context_in_struct_xls_type,
617620
/*initial_values=*/{},
618-
/*fifo_config=*/xls::FifoConfig{.depth = 0},
621+
/*fifo_config=*/
622+
xls::FifoConfig(/*depth=*/0, /*bypass=*/true,
623+
/*register_push_outputs=*/false,
624+
/*register_pop_outputs=*/false),
619625
xls::FlowControl::kReadyValid));
620626
}
621627
IOChannel new_channel;

Diff for: ‎xls/contrib/xlscc/translator.cc

+6-2
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
#include <map>
2424
#include <memory>
2525
#include <optional>
26-
#include <ostream>
2726
#include <regex> // NOLINT
2827
#include <set>
2928
#include <sstream>
@@ -80,11 +79,13 @@
8079
#include "xls/ir/ir_parser.h"
8180
#include "xls/ir/nodes.h"
8281
#include "xls/ir/op.h"
82+
#include "xls/ir/package.h"
8383
#include "xls/ir/source_location.h"
8484
#include "xls/ir/type.h"
8585
#include "xls/ir/value.h"
8686
#include "xls/ir/value_utils.h"
8787
#include "xls/solvers/z3_utils.h"
88+
#include "../z3/src/api/z3_api.h"
8889
#include "re2/re2.h"
8990

9091
using std::list;
@@ -4710,7 +4711,10 @@ absl::StatusOr<CValue> Translator::GenerateIR_LocalChannel(
47104711
xls::Channel * xls_channel,
47114712
package_->CreateStreamingChannel(
47124713
ch_name, xls::ChannelOps::kSendReceive, item_type_xls,
4713-
/*initial_values=*/{}, /*fifo_config=*/xls::FifoConfig{.depth = 0},
4714+
/*initial_values=*/{}, /*fifo_config=*/
4715+
xls::FifoConfig(/*depth=*/0, /*bypass=*/true,
4716+
/*register_push_outputs=*/false,
4717+
/*register_pop_outputs=*/false),
47144718
xls::FlowControl::kReadyValid));
47154719

47164720
unused_xls_channel_ops_.push_back({xls_channel, /*is_send=*/true});

Diff for: ‎xls/dslx/ir_convert/proc_config_ir_converter.cc

+5-2
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,11 @@ absl::Status ProcConfigIrConverter::HandleChannelDecl(const ChannelDecl* node) {
144144
// as they avoid many issues, especially wrt combo-loops and scheduling.
145145
// TODO: google/xls#1391 - we should have a better way to specify fifo
146146
// configuration.
147-
fifo_config.emplace(
148-
FifoConfig{.depth = *fifo_depth, .bypass = *fifo_depth == 0});
147+
fifo_config.emplace(FifoConfig(
148+
/*depth=*/*fifo_depth,
149+
/*bypass=*/true,
150+
/*register_push_outputs=*/*fifo_depth == 0,
151+
/*register_pop_outputs=*/false));
149152
}
150153
XLS_ASSIGN_OR_RETURN(
151154
StreamingChannel * channel,

Diff for: ‎xls/ir/block_elaboration_test.cc

+7-4
Original file line numberDiff line numberDiff line change
@@ -230,10 +230,13 @@ absl::StatusOr<Block*> BlockWithFifoInstantiation(Package& p) {
230230
BValue a = bb.InputPort("a", u32);
231231
BValue b = bb.InputPort("b", u32);
232232

233-
XLS_ASSIGN_OR_RETURN(
234-
FifoInstantiation * fifo_inst,
235-
bb.block()->AddFifoInstantiation(
236-
"fifo_inst", FifoConfig{.depth = 1, .bypass = true}, u32));
233+
XLS_ASSIGN_OR_RETURN(FifoInstantiation * fifo_inst,
234+
bb.block()->AddFifoInstantiation(
235+
"fifo_inst",
236+
FifoConfig(/*depth=*/1, /*bypass=*/true,
237+
/*register_push_outputs=*/true,
238+
/*register_pop_outputs=*/false),
239+
u32));
237240

238241
BValue lit1 = bb.Literal(UBits(1, 1));
239242
bb.InstantiationInput(fifo_inst, "push_data", a);

Diff for: ‎xls/ir/channel.cc

+30-3
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,29 @@
3434
#include "xls/ir/value_utils.h"
3535

3636
namespace xls {
37+
FifoConfig::FifoConfig(int64_t depth, bool bypass, bool register_push_outputs,
38+
bool register_pop_outputs)
39+
: depth_(depth),
40+
bypass_(bypass),
41+
register_push_outputs_(register_push_outputs),
42+
register_pop_outputs_(register_pop_outputs) {}
43+
44+
FifoConfigProto FifoConfig::ToProto(int64_t width) const {
45+
FifoConfigProto proto;
46+
proto.set_width(width);
47+
proto.set_depth(depth_);
48+
proto.set_bypass(bypass_);
49+
proto.set_register_push_outputs(register_push_outputs_);
50+
proto.set_register_pop_outputs(register_pop_outputs_);
51+
return proto;
52+
}
53+
54+
std::string FifoConfig::ToString() const {
55+
return absl::StrFormat(
56+
"FifoConfig{ depth: %d, bypass: %d, register_push_outputs: %d, "
57+
"register_pop_outputs: %d }",
58+
depth_, bypass_, register_push_outputs_, register_pop_outputs_);
59+
}
3760

3861
std::string ChannelKindToString(ChannelKind kind) {
3962
switch (kind) {
@@ -83,9 +106,13 @@ std::string Channel::ToString() const {
83106
const std::optional<FifoConfig>& fifo_config =
84107
streaming_channel->fifo_config();
85108
if (fifo_config.has_value()) {
86-
absl::StrAppendFormat(&result, "fifo_depth=%d, bypass=%s, ",
87-
fifo_config->depth,
88-
fifo_config->bypass ? "true" : "false");
109+
absl::StrAppendFormat(
110+
&result,
111+
"fifo_depth=%d, bypass=%s, "
112+
"register_push_outputs=%s, register_pop_outputs=%s, ",
113+
fifo_config->depth(), fifo_config->bypass() ? "true" : "false",
114+
fifo_config->register_pop_outputs() ? "true" : "false",
115+
fifo_config->register_push_outputs() ? "true" : "false");
89116
}
90117
}
91118

0 commit comments

Comments
 (0)