[Handshake] Support for extra signals in operations #225

shundroid · 2024-12-22T21:15:41Z

We needed additional changes in handshake operations to fully utilize the extra signals in the circuit. The changes in this PR are general and not specific to speculation.

Changes:

ConstantOp now receives ControlType (possibly with extra signals) and propagates it to the result.
- Therefore, it remains constant only for data, consider renaming it to ConstantDataOp?
MuxOp and CMergeOp accept variadic inputs with "uneven" extra signals.
- Refer to the unit tests for the expected behavior.
To make the type system complete and sound:
- Added constraints like AllDataTypesMatch, AllExtraSignalsMatch, AllDataTypesMatchWithinVariadic, MergingExtraSignals, etc.
- Applied these constraints to MemPortOp (LoadOp, StoreOp), ControlMergeOp, MuxOp, ConstantOp
Fixed bug on handling extra signals in CmpIOp and CmpFOp
Unit tests

I think some implementation might be difficult to understand (especially MergingExtraSignals constraint), though I tried to add as many comments as I can.
Please feel free to let me know if something is unclear.

shundroid · 2024-12-22T21:21:12Z

include/dynamatic/Dialect/Handshake/HandshakeInterfaces.td

-    mlir::Type resultType = $_op->getResult(0).getType();
-
-    for (auto operand : operands)
-      if (operand.getType() != resultType)
-        return concreteOp.emitOpError("operand has type ") << operand.getType()
-            << ", but result has type " << resultType;
-


This type checking between operands and result is (no longer) unnecessary here.
For CMerge and Mux, the type is no longer the same between operands and result.
For MergeOp, SameOperandsAndResultType is already constrained

Does it make sense to check if the operands' data fields all have the same type? I guess we still want to avoid situations like channel<i32, [spec : i1]> and channel

Update: Now I see that you have some other constraints to check that

shundroid · 2024-12-22T22:25:20Z

@lucas-rami (CC @murphe67 ) Thank you for reviewing this pull request.
I want to discuss some things with you.

(1) New assembly format for mux / control_merge

The extra signals of the inputs of these operations can now be uneven, so we need to explicitly specify all input types in the IR. For better readability, I propose the following format:

%data = mux %select [%data0, %data1] : <i1>, [<i32>, <i32, [spec: i1]>] to <i32, [spec: i1]>
%data, %index = control_merge [%data0, %data1] : [<i32>, <i32, [spec: i1]>] to <i32, [spec: i1]>, <i1>

Let me know your thoughts on this. Once we agree, I’ll update the rest of the existing unit tests accordingly.

(2) PredOpTraits like AllExtraSignalsMatch vs OpInterfaces like SameExtraSignalsInterface

I noticed you’ve implemented OpInterfaces like SameExtraSignalsInterface to constrain extra signals. However, I’ve implemented a similar approach using PredOpTraits and prefer it for the following reasons:

PredOpTraits work well for constraints applied multiple times. For example, AllDataTypesMatch is applied twice in MemPortOp for both address and data. This is more intuitive and easier to represent with PredOpTraits.

dynamatic/include/dynamatic/Dialect/Handshake/HandshakeOps.td

Lines 746 to 752 in b94f744

    
           class Handshake_MemPortOp< 
        
               string mnemonic, list<Trait> customTraits, list<OpBuilder> customBuilders> : 
        
             Handshake_Op<mnemonic, customTraits # [ 
        
               MemPortOpInterface, 
        
               AllDataTypesMatch<["address", "addressResult"]>, 
        
               AllDataTypesMatch<["data", "dataResult"]>, 
        
               DeclareOpInterfaceMethods<NamedIOInterface, ["getOperandName", "getResultName"]>

With PredOpTraits, the constraints are explicit in the operation definition, making them clearer (which inputs/outputs are involved) and more declarative.
OpInterfaces like SameExtraSignalsInterface don’t align well with the general concept of "interfaces."

I'd like to hear your opinion on this approach.

murphe67

Thanks for all the hard work Shun!

I would love a nice markdown file describing the type constraints on each operation we can add to the documentation folder?

Other than that I have a couple comments on function names, variables and comments. :)

include/dynamatic/Dialect/Handshake/HandshakeArithOps.td

include/dynamatic/Dialect/Handshake/HandshakeOps.td

include/dynamatic/Dialect/Handshake/HandshakeTypeInterfaces.td

include/dynamatic/Dialect/Handshake/HandshakeTypes.td

lib/Dialect/Handshake/HandshakeOps.cpp

include/dynamatic/Dialect/Handshake/HandshakeTypes.td

lib/Dialect/Handshake/HandshakeOps.cpp

shundroid · 2025-01-24T16:19:02Z

@murphe67 I added a document, so I'd be glad if you could review it as well.
I noticed some conversations are not resolved, I'll work on them

murphe67

I absolutely love the document Shun, thank you! I have some superficial opinions, as always ahahahaha.

docs/ExtraSignalsForOperations.md

…-extra-signals

shundroid · 2025-02-07T14:31:33Z

@AyaElAkhras In the latest commit, I updated the newly created document for this PR to include the implementation description in Section 2. It's still a draft, so the structure or details might change, but the information is mostly finalized. Please take a look, and feel free to share any suggestions!

Link: https://github.com/EPFL-LAP/dynamatic/blob/ops-with-extra-signals/docs/ExtraSignalsForOperations.md

shundroid · 2025-02-07T16:11:21Z

include/dynamatic/Dialect/Handshake/HandshakeOps.td

+def StoreOp : Handshake_MemPortOp<"store", [
+  AllExtraSignalsMatch<["address", "data"]>,
+  // In StoreOp, addressResult and dataResult are connected to a memory controller.
+  IsSimpleHandshake<"addressResult">,
+  IsSimpleHandshake<"dataResult">
+], []> {


I realized StoreOp is usually not inside a speculative region (or an out-of-order region I guess). Should I just constrain all the operands/results to be simple?

Depends on if @AyaElAkhras has tagged stores?

In my old implementation, I had a somehow pointless tagged store that takes extra tag ports associated with the address and data but does nothing with them internally because the MC and LSQ (so far) do not have support for tags. I only added them to make the interface consistent (when the components calculating the address and data are tagged).

So we have two options: 1) enforce that tags cannot reach stores while we work on speculation, since it should not happen under our current implementation, and then you can edit the validation to what actually makes sense for you in the new implementation or 2) allow tags to reach stores now, and assume the same kind-of-pointless tagged stores will exist.

@shundroid, could we add a validation that specifically spec tags should not reach stores somehow? maybe in the algorithm that adds the spec tag to the IR, but not part of the core validation system?

On the other hand, if we add it to the core validation system, if anyone runs an optimization pass after speculation, they will know if they have broken speculation or not, which may be a good thing.

could we add a validation that specifically spec tags should not reach stores somehow? maybe in the algorithm that adds the spec tag to the IR, but not part of the core validation system?

Yes, it's easy. I'll add it.

when the components calculating the address and data are tagged

@AyaElAkhras Does this happen when the store op is placed between the tagger and untagger but after the aligner (as described in the paper)?

If store ops must be in the "tagged" region, I'd go with Emmet's option 2), even if the tags are ineffective. But if the data and address operands can be easily made tagless, I'd prefer option 1).

On the other hand, if we add it to the core validation system, if anyone runs an optimization pass after speculation, they will know if they have broken speculation or not, which may be a good thing.

For this use case, maybe we want to add a trait like OnlyOutsideSpecRegion in the future to ensure that none of the operands have spec tag (if we take option 12 for now).

I think option 2 with the check inside the algorithm makes the most sense for now, but am pretty open. 😄

Sorry guys, I got swamped with a completely unexpected issue. I hope it gets resolved by tonight and I will study what I missed then and let you know.

I finally managed to process this (apologies again for the delay!). From the document, I guess you ended up with option 2)? If yes, it aligns with my old implementation, so I'm happy :D

@AyaElAkhras Does this happen when the store op is placed between the tagger and untagger but after the aligner
(as described in the paper)?

Yes.

AyaElAkhras · 2025-02-07T21:42:38Z

@AyaElAkhras In the latest commit, I updated the newly created document for this PR to include the implementation description in Section 2. It's still a draft, so the structure or details might change, but the information is mostly finalized. Please take a look, and feel free to share any suggestions!

Link: https://github.com/EPFL-LAP/dynamatic/blob/ops-with-extra-signals/docs/ExtraSignalsForOperations.md

Thanks a lot @shundroid! I will process it as soon as I can and get back to you.

shundroid · 2025-02-10T06:47:48Z

(Maybe just me) I needed to read the code to understand what DataTypesMatchWithVariadicFront<string variadic, string nonvariadic>

AllDataTypesMatchWithinVariadic and DataTypesMatchWithVariadicFront were only used internally to define AllDataTypesMatchWithVariadic.

I realized this was confusing, so I updated the implementation to define only AllDataTypesMatchWithVariadic independently.

shundroid · 2025-02-10T06:57:10Z

I noticed some operations in HandshakeArithOps.td still use SameExtraSignalsInterface (as I mentioned here).

Since this PR already has a large diff, I'll update those operations and remove SameExtraSignalsInterface in a follow-up PR.

pcineverdies · 2025-02-10T08:08:15Z

As far as my opinion matters, I think the doc file you provided is a great source of information! Thanks for the work :)

AyaElAkhras · 2025-02-12T19:30:03Z

Sorry guys, I got swamped with a completely unexpected issue. I hope it gets resolved by tonight and I will study what I missed then and let you know.

AyaElAkhras · 2025-02-14T17:41:32Z

Thanks a lot @shundroid and @murphe67 for the clear and nice presentation and for all the effort!

I processed the document and left a few clarification questions and suggestions. Other than those, I think everything is perfect :)

shundroid · 2025-02-14T18:28:39Z

Thank you, @AyaElAkhras ! I can't see your comments, are they still in draft? Could you click the Submit Review button?

AyaElAkhras · 2025-02-14T16:20:14Z

docs/ExtraSignalsForOperations.md

+
+- The data output includes an extra signal `A` if, and only if, at least one of the inputs carries the extra signal `A`.
+
+The selector input (for Mux) or the output (for CMerge) is kept simple, meaning it does not carry any extra signals.


This also caught my attention. In FPGA'24, we have a tagged Mux and a tagged CMerge, and for those, the select carries a tag signal as well. If I understood it right, this will not currently work, right? How hard it is to make it work?

AyaElAkhras · 2025-02-14T16:28:30Z

docs/ExtraSignalsForOperations.md

+
+The data output has `spec: i1` and `tag: i8` because some inputs have them, and nothing else.
+
+The specification for the output extra signals implies that if an input is selected but lacks a specific extra signal present in other inputs, the Mux or CMerge must provide the value of the missing extra signal for the output.


Just to make sure I understand what this means:

If the output of the Mux has an additional tag signal and in0 of the Mux does not have this signal and is chosen to be passed to the output, then the Mux will have some default value to put in the output tag signal?

I guess in [TypeSystem] Converting untagged values to default tag #226 we were also speaking of Source nodes, does this apply to them as well?

Yes- exactly what values the mux/cmerge/source provides will be covered in the signal manager prs, like #274

AyaElAkhras · 2025-02-14T16:30:55Z

docs/ExtraSignalsForOperations.md

+
+### MuxOp and CMergeOp
+
+These operations may have different extra signals for each input because they typically reside at the boundary of a basic block, receiving inputs from various blocks. For instance, the extra signals on the inputs of a MuxOp might look like this:


I agree that Muxes and CMerges are special in that sense, but Merges are also similar to them. We may one day want to have this flexibility for Merges as well— is there a reason not to extend it to Merges?

No, these rules are super flexible/editable. We currently do not do it intentionally, so the validation system prevents it.

The key with this system is to have a clear document with all the rules and reasons, but the PR more contributes the system and a first set of rules.

AyaElAkhras · 2025-02-14T16:58:32Z

docs/ExtraSignalsForOperations.md

+
+More on operation arguments: https://mlir.llvm.org/docs/DefiningDialects/Operations/#operation-arguments
+
+Each operation also has **results**, which represent the outputs of the RTL here. For instance, `ConditionalBranchOp` has two results, corresponding to the "true" and "false" branches.


We could support a variable number of results too (i.e., variadic results of MLIR), right? For instance, I may want to add a Branch with a variable number of outputs not just 2.

Yes, fork is an example. I'll clarify this in the document tomorrow

AyaElAkhras · 2025-02-14T17:22:20Z

docs/ExtraSignalsForOperations.md

+- `MergingExtraSignals` – Validates extra signal consistency across the data inputs and data output.
+- `AllDataTypesMatchWithVariadic` – Ensures uniform data types across the data inputs and data output.
+
+Additionally, the `selector` port is of type `SimpleChannel`, as it does not carry extra signals.


But, this can be problematic (see my earlier question). Which type of constraint is enforced here? And, can we choose not to enforce it?

SimpleChannel is kind of a constraint here, to enforce the channel is without any extra signals.
Yes, it's easy to enforce/not enforce these constraints

AyaElAkhras · 2025-02-14T17:23:49Z

docs/ExtraSignalsForOperations.md

+The following constraints ensure proper handling of extra signals:
+
+- `MergingExtraSignals` – Validates extra signal consistency across the data inputs and data output.
+- `AllDataTypesMatchWithVariadic` – Ensures uniform data types across the data inputs and data output.


How different is this from AllDataTypesMatch? Is the former (AllDataTypesMatch) not supporting variable number of inputs/outputs?

Yes, you're correct. I'll emphasize that (across the data inputs and variadic data output), thanks!

AyaElAkhras · 2025-02-14T17:37:00Z

docs/ExtraSignalsForOperations.md

+
+Next, we’ll take a closer look at how these rules are implemented. We’ll begin by introducing some fundamental concepts.
+
+### Operations


It is not clear to me what kind of constraints do you impose on Forks? They may or may not fall under 'operations within a basic block'. Regardless, I may want a two-output Fork where the input consists of an i32 data signal along with an extra tag signal, and one output should contain only the tag signal, while the other should consist of the i32 data signal.

Can this work, or will it violate some constraint?

Currently this would violate the constraint, since we don't have a way to generate that kind of a fork.

We have this exact situation in speculation, and we are currently planning to add conversion operations under the relevant fork outputs- your suggestion could be used instead, so I might ask Lana what she thinks about it.

(I guess my answer is again: we implemented rules which correspond to speculation, but the goal of this PR is really that whoever needs to change the rules is able to easily)

Got it, thanks @murphe67! :)

AyaElAkhras · 2025-02-14T17:39:04Z

include/dynamatic/Dialect/Handshake/HandshakeOps.td

+def StoreOp : Handshake_MemPortOp<"store", [
+  AllExtraSignalsMatch<["address", "data"]>,
+  // In StoreOp, addressResult and dataResult are connected to a memory controller.
+  IsSimpleHandshake<"addressResult">,
+  IsSimpleHandshake<"dataResult">
+], []> {


I finally managed to process this (apologies again for the delay!). From the document, I guess you ended up with option 2)? If yes, it aligns with my old implementation, so I'm happy :D

@AyaElAkhras Does this happen when the store op is placed between the tagger and untagger but after the aligner
(as described in the paper)?

Yes.

AyaElAkhras · 2025-02-14T18:44:00Z

Thank you, @AyaElAkhras ! I can't see your comments, are they still in draft? Could you click the Submit Review button?

Opps, sorry, just submitted!

shundroid commented Dec 22, 2024

View reviewed changes

shundroid marked this pull request as ready for review December 22, 2024 22:20

shundroid requested review from lucas-rami and murphe67 December 22, 2024 22:20

shundroid mentioned this pull request Dec 22, 2024

[Speculation] Updated HandshakeOps to handle spec tags #206

Draft

murphe67 reviewed Dec 23, 2024

View reviewed changes

shundroid commented Dec 23, 2024

View reviewed changes

include/dynamatic/Dialect/Handshake/HandshakeTypes.td Outdated Show resolved Hide resolved

murphe67 reviewed Dec 23, 2024

View reviewed changes

include/dynamatic/Dialect/Handshake/HandshakeTypes.td Outdated Show resolved Hide resolved

murphe67 reviewed Dec 23, 2024

View reviewed changes

lib/Dialect/Handshake/HandshakeOps.cpp Outdated Show resolved Hide resolved

shundroid force-pushed the ops-with-extra-signals branch 3 times, most recently from 22eefaf to 575fd80 Compare January 24, 2025 16:16

shundroid removed the request for review from lucas-rami January 24, 2025 16:19

murphe67 reviewed Jan 24, 2025

View reviewed changes

shundroid added 14 commits January 25, 2025 00:49

support control with extra signals in ConstantOp

87f6535

fixed cmpi/fop

bba4f01

added AllDataTypesMatch and AllExtraSignalsMatch

cceadbd

constrained ConstantOp (with pos/neg unit tests)

750a3ff

constrained MemPortOp (LoadOp/StoreOp)

c589bf8

updated mux and cmerge

e8ddc91

updated comments on constraints

9e4735d

defined MergingExtraSignals

d8b6f9e

removed SameExtraSignalsInterface from Mux/CMerge

e55c5df

added Variadic constraints

9ab2377

removed temp default constructor of ExtraSignal

5e89ce1

use AllDataTypesMatchWithVariadic

68a9a88

updated to replaceExtraSignals

7ab0f65

fixed comments

4232e3f

shundroid and others added 10 commits February 7, 2025 12:26

added doc

205dbbc

added example on simple types

f2900a6

use SimpleType in Mux/CMerge instead of IsSimpleHandshake trait

994f8fa

Make non-pull-request specific

d625ca2

updated document

6bf372e

added Section 2

0b333ce

minor changes to the doc

ee0bb45

trait explanations at last

15c9445

removed temp doc

323dca3

Merge branch 'dev/shundroid/ops-with-extra-signals-doc' into ops-with…

a222ab6

…-extra-signals

shundroid added 3 commits February 7, 2025 15:37

fixed ConditionalBranchOp

e577cf4

fixed example CondBr -> Compare

327c40b

revert ConditionalBranchOp to use AllTypesMatch

0c5be47

shundroid commented Feb 7, 2025

View reviewed changes

shundroid added 2 commits February 10, 2025 07:35

simple implementation of AllDataTypesMatchWithVariadic

7789fef

added unit tests

25c090d

shundroid added 2 commits February 10, 2025 13:15

updated comments for SameOperandsAndResultType

623f00b

updated ConstantOp section of the doc

ab809b9

AyaElAkhras reviewed Feb 14, 2025

View reviewed changes

shundroid added 2 commits February 15, 2025 14:13

mentioned variadic results

a75443b

clarify variadic data output

ca31e9e


		- The data output includes an extra signal `A` if, and only if, at least one of the inputs carries the extra signal `A`.

		The selector input (for Mux) or the output (for CMerge) is kept simple, meaning it does not carry any extra signals.


		The data output has `spec: i1` and `tag: i8` because some inputs have them, and nothing else.

		The specification for the output extra signals implies that if an input is selected but lacks a specific extra signal present in other inputs, the Mux or CMerge must provide the value of the missing extra signal for the output.


		### MuxOp and CMergeOp

		These operations may have different extra signals for each input because they typically reside at the boundary of a basic block, receiving inputs from various blocks. For instance, the extra signals on the inputs of a MuxOp might look like this:


		More on operation arguments: https://mlir.llvm.org/docs/DefiningDialects/Operations/#operation-arguments

		Each operation also has results, which represent the outputs of the RTL here. For instance, `ConditionalBranchOp` has two results, corresponding to the "true" and "false" branches.


		Next, we’ll take a closer look at how these rules are implemented. We’ll begin by introducing some fundamental concepts.

		### Operations

[Handshake] Support for extra signals in operations #225

Are you sure you want to change the base?

[Handshake] Support for extra signals in operations #225

Conversation

shundroid commented Dec 22, 2024 • edited Loading

shundroid Dec 22, 2024 • edited Loading

Choose a reason for hiding this comment

Jiahui17 Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

shundroid commented Dec 22, 2024

murphe67 left a comment

Choose a reason for hiding this comment

shundroid commented Jan 24, 2025

murphe67 left a comment

Choose a reason for hiding this comment

shundroid commented Feb 7, 2025 • edited Loading

shundroid Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murphe67 Feb 9, 2025 • edited Loading

Choose a reason for hiding this comment

shundroid Feb 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AyaElAkhras commented Feb 7, 2025

shundroid commented Feb 10, 2025

shundroid commented Feb 10, 2025 • edited Loading

pcineverdies commented Feb 10, 2025

AyaElAkhras commented Feb 12, 2025

AyaElAkhras commented Feb 14, 2025

shundroid commented Feb 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murphe67 Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murphe67 Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AyaElAkhras commented Feb 14, 2025

shundroid commented Dec 22, 2024 •

edited

Loading

shundroid Dec 22, 2024 •

edited

Loading

Jiahui17 Feb 6, 2025 •

edited

Loading

shundroid commented Feb 7, 2025 •

edited

Loading

shundroid Feb 7, 2025 •

edited

Loading

murphe67 Feb 9, 2025 •

edited

Loading

shundroid Feb 9, 2025 •

edited

Loading

shundroid commented Feb 10, 2025 •

edited

Loading

murphe67 Feb 14, 2025 •

edited

Loading

murphe67 Feb 14, 2025 •

edited

Loading