[magancarz]

This PR adds a warning for the channel operations token mismatch which is thrown in case of impossibility to correctly assemble data used in send based on the operation tokens. This is handled by tracking token and data dependencies in semantics_analysis.cc with DependencyOrderCollector. Each node is assigned a dependency level based on nodes it gets its values from. In case of aliases, data combining or data processing it stays the same, but the levels increments after every IO operation. With the collected dependency data, the final check can be simply performed by comparing dependency levels of send operation tokens and data values.

For example, token used in the second send here can indicate that a value is sent before it is received.

...
let tok0 = join();
let (tok1, data0) = recv(tok0, req0_r);
let tok2 = send(tok1, resp0_s, data0); // correct token
let (tok3, data1) = recv(tok2, req1_r);
let tok2 = send(tok1, resp1_s, data1); // wrong token
...

With changes from this pull request, it will throw the following warning during the type check stage:

let (tok3, data1) = recv(tok2, req1_r);
let tok2 = send(tok1, resp1_s, data1); // wrong token
~~~~~~~~~~~~~~~^--------------------^ Operation `send(tok1, resp1_s, data1)` has an IO token ordering mismatch

[ericastor]

Just to clarify the intent here - the goal is to warn if there is a data dependency that is not reflected in the token graph?

(Personally, I'm not sure that's a goal of DSLX, so I'll let the language owners weigh in on that one.)

[magancarz]

The goal was to warn users about situations where there is a mismatch in channel operations tokens order which is important for scheduling, e.g.:

...
let tok0 = join();
let (tok1, data0) = recv(tok0, req0_r);
let tok2 = send(tok1, resp0_s, data0); // correct token
let (tok3, data1) = recv(tok2, req1_r);
let tok2 = send(tok1, resp1_s, data1); // wrong token
...

Token used in the second send here can indicate that a value is sent before it is received.

DependencyOrderCollector tracks the dependency levels of tokens and data nodes, so semantics_analysis can perform a check in a post type check pass if tokens used in send allow the data to be received and processed before the operation is scheduled.

Edit: Added this example to the description.

[proppy]

Token used in the second send here can indicate that a value is sent before it is received.

@magancarz afaik the current behavior (albeit poorly undocument) of the toolchain is to take the union (like join()) of the all the token and dataflow dependencies to resolve ordering.

Personally, I'm not sure that's a goal of DSLX

This is definitly an area where best practices are unclear, should you:

• a) strictly reflect the data dependencies w/ the token graph (what this PR enforces)
• b) treat token as optional, and use token() or join() when the ordering can be obviously inferred from the dataflow.

[magancarz]

I've rebased changes on latest main, so Continuous Integration job will now succeed.

[magancarz]

The current behavior (albeit poorly undocument) of the toolchain is to take the union (like join()) of the all the token and dataflow dependencies to resolve ordering.

@proppy let me clarify. By the current behavior you referred to actions of some part of the toolchain before this pull request or the idea of comparing token and data dependencies passed to send operation introduces with these changes?

[proppy]

By the current behavior you referred to actions of some part of the toolchain before this pull request

Sorry for the lack of context, I was implicitly referring to described in https://google.github.io/xls/tutorials/dataflow_and_time/ which states that:

if operation Y depends on the result of operation X, this is represented by an edge from X to Y [...] these operations will both take and produce a token, which can be used to add dependency edges.

which I interpret as (but @ericastor and others can correct me):

• when the scheduler resolve ordering constraints: it treats all node operands (being data or token) as equal dependencies.

See predecessors in:

xls/xls/scheduling/schedule_graph.cc

Lines 139 to 140 in 07ce6ae

	.predecessors = std::vector<Node*>(node->operands().begin(),
	node->operands().end()),

With that in mind, I think what we're trying to fix (maybe that should be spelled out in dedicated enhancement proposal to discuss it further?) is that there is no current "best practices" on "what to do with token" when the dataflow is enough to determinate the ordering of operations:

• a) should we try to mirror the dataflow graph with the token graph (as enforced in this PR)?
• b) use dummy token to represente the lack of additional dependencies?

Both would result in the same schedule (since we just take the union of the operands as predecessors: a) just mirrors the existing dependencies" so doesn't add anything new and b) is just adding noops).

But enforcing one or the other as best practice would create a significantly different developer experience when writing and reading DSLX code:

• a) DSLX writers do extra work to make the token graph match the dataflow so that DSLX readers always get a coherent picture of ordering from the token graph
• b) DSLX writers can use placeholder tokens to default on dataflow ordering at the expense of DSLX readers having to figure out when tokens carry (or not) semantic value.

IMO, the current experience which is an inconsistent mix of a) and b) is not doing ourselves any favors, as it creates an impedance missmatch between DSLX writers and readers depending of their taste (or literacy).

[ericastor]

My answer is that DSLX represents dependency through dataflow. For the cases where the dependency is meaningful but doesn't take the form of required input data, DSLX expresses this with tokens - and I think this is a principled choice.

In other words, neither redundant nor "missing" tokens are a problem. Tokens express a semantic fact. I don't believe it's possible for the compiler to tell what semantics exist outside of the code as written, so I don't think a warning in either direction is a good idea.

My personal thoughts on best practices can be summed up as:

• Tokens should be threaded between operations when there's a non-data dependency... even if there's also a data dependency.

From a practical perspective, this means that if the designer wants externally-visible operation 2 to never occur before externally-visible operation 1, then there should be a token threaded from operation 1 to operation 2, regardless of whether there's a data dependency between them. If the order is not part of their intent, then they shouldn't thread a token between these operations; if there's a data dependency, XLS will resolve this during scheduling.

Also, yes, I believe that the fact that tokens are a non-optional parameter on externally-visible operations is not ideal for this intent. We keep going back and forth internally on what the syntax should be to let the user omit tokens where they're not wanted. I have opinions on most of the options that have been proposed, but so do others.

---

But - I'm just one member of this team, and I agree we should agree on a consistent answer.

[rw1nkler]

@ericastor, you said that the original intent was for tokens to represent non-data dependencies.
I agree that this semantics is not reflected in the current syntax of the language. If it were, I would say that the example from the description would become:

let (_, data0) = recv(join(), req0_r);
let tok = send(join(), resp0_s, data0);
let (_, data1) = recv(tok, req1_r);
let tok2 = send(join(), resp1_s, data1);

Unfortunately, the checks that we provided in this PR suggest that this meaning of tokens is rarely used in the public codebase. Here are some examples that seems to violate this semantics of tokens:

• https://github.com/google/xls/blob/main/xls/examples/apfloat_fmac.x#L46
• https://github.com/google/xls/blob/main/xls/examples/xls_pipeline.x#L90
• https://github.com/google/xls/blob/main/xls/examples/serialized_decomposer.x#L44
• https://github.com/google/xls/blob/main/xls/examples/proc_network.x#L44
• https://github.com/google/xls/blob/main/xls/examples/passthrough.x#L30
• https://github.com/google/xls/blob/main/xls/examples/memory_proc.x#L47
• https://github.com/google/xls/blob/main/xls/examples/lfsr_proc.x#L43
• https://github.com/google/xls/blob/main/xls/examples/custom_schedule.x#L36

As of today, we end up with two parallel dependency graphs - data and token - both of which must be considered to understand I/O ordering. Aligning token dependencies with data dependencies where they overlap may simplify reasoning about the ordering of operations and encourage a more consistent style of DSLX code, without requiring changes to the language syntax.

Please treat this as just another argument in a discussion, so a better final decision can be made. I agree that the original intent that @ericastor stated is a clean version of describing non-data dependencies. At the same time, if this meaning of tokens is taken, there is not much that we can do to check correctness of user's code and someone can mix non-data and data deps without noticing.

I also agree that the final goal that we have, for removing redundancy and making tokens optional will make the code cleaner, and will remove inconsistency in the codebase.

[magancarz]

I've simplified disabling multiple warnings at once from a WarningKindSet and disabled kIOOrderingMismatch warning by default.

[magancarz]

I've rebased changes onto the latest main to resolve conflicts in semantics_analysis.

[magancarz]

@richmckeever thank you for the review! I've applied suggestions as described in my replies, moved dependency collector declarations from header to source file and fixed bug in AddNodeAlias/AddNodeDependency functions related to taking a reference of a map item.

[magancarz]

I've updated latest push with the dependency collector object rename to preserve the naming convention of non-member variables.

[proppy]

can you rebase?

[magancarz]

@proppy sure. I've rebased pull request changes onto the latest main and resolved conflicts.

[proppy]

@ericastor @richmckeever how does this map to the recent discussions we had on best practices around tokens?

do we still want to mecanism to flag those as warnings (disabled by default) as pointed in:
#3895 (comment)

[richmckeever]

Is the warning requiring you to add the named tokens here and below, or is it an independent choice?

Some time after you started working on this, there was an internal discussion on best practices, where we concluded that it's better not to name tokens when it's not required to satisfy externally visible I/O ordering requirements (e.g. an outside component needs you to do 2 reads serially).

[rw1nkler]

This changes were required by the warning. If we denote the dependency relation with <-, you can see that:

buf_data (IO) <- recv_sb_output <-  state <- ctrl_valid (IO)

So there is a data dependency that was previously not expressed via tokens.

[magancarz]

I've rebased changes to resolve merge conflicts.

We expect ZSTD job to fail as there is an issue with benchmark_synth targets described in bazel_rules_hdl#440.

[proppy]

@richmckeever did we reach consensus on whether we want to land this?

[proppy]

marking this as draft until we reach consensus if we want to land this or not, @magancarz maybe we can revive this discussion on a new https://github.com/google/xls/issues