[Handshake] Add booleans to mem dependence attr

[rpirayadi]

Marking dependencies as active and inactive

As previously suggested here by @paolo-ienne, We decided to mark dependencies as active and inactive (for more detailed information also take a look at other comments like this). As suggested here by @lucas-rami, I have added them to memDependeceAttr itself by adding a boolean named isActive. Also for more readability, they are written in IR as "active" and "inactive" instead of true and false. An example of a modified version of load operation is below.

%addressResult, %dataResult = load[%17] %outputs {handshake.bb = 2 : ui32, handshake.deps = #handshake<deps[<"store0" (2) "inactive" [[0, 0]]>]>, handshake.mem_interface = #handshake.mem_interface<MC>, handshake.name = "load0"} : <i2>, <i32>

Changing HandshakeAnalyzeLSQUsage to HandshakeInactivateEnforcedDeps

In order to make use of these booleans, I have updated the pass HandshakeAnalyzeLSQUsage and renamed it to HandshakeInactivateEnforcedDeps. The logic of the code more or less stayed the same, with one big difference. HandshakeInactivateEnforcedDeps inactivates a dependency even if it doesn't mean we can connect the port to the Memory Controller instead of LSQ. For example, if one operation has three dependencies and only one of them can be inactivated, it is inactivated, however, it still remains connected to the LSQ. I've also fixed some issues in the comments about GIID. GIID is a short form for Globally Instruction In-order Dependent, which in some comments was mistakenly written as Globally In-order InDependent. I think this mistake was made due to the fact that operations that didn't have enforced dependencies, due to data dependency, and it was still needed to honor the dependency was called dependent. This made a mess in naming so, I have replaced this with opsWithNonEnforcedDeps.

Testing

Since this modifies the IR, I wanted to ensure nothing breaks. So, I ran the unit test and it's passing. I've also ran all the integration tests and all of them are passing except for the ones below (two of them are timed out). I've checked and these tests are also failing for me on the main branch, so I think the modifications are fine.

1. cordic
2. correlation_float
3. covariance_float
4. external_integration
5. symm_float
6. trisolv
7. matching_2
8. complexdiv
9. path_profiling
10. kmp
11. covariance
12. cnn
13. admm
14. dct
15. while_loop_2

[pcineverdies]

Thanks for your contribution!
I just addressed a couple of style problems for the sake of giving a review :)
Wait for Lucas to get a real review!

[pcineverdies]

I think it's advisable to declare a variable as close as possible to its usage - to minimise its scope. You don't use this variable outside of the for body.

[pcineverdies]

This is just a thought: is it advisable to have such a long text rather than a single character? (i.e. either A or I or whatever). For instance, in the buffering context, the properties of the buffers are expressed with a single letter, and this does not lack of readability as long as you are aware of their meaning.
The main consequence would be to have a lighter IR rather than a large one (considering that load and store operations are already pretty ugly...).

However, Lucas is probably more aware of the different implications here.

[rpirayadi]

@pcineverdies Thanks for your comments! :)
I've fixed them all except the last one.
I personally prefer better readability, but I understand that someone who is reading the IR should already be knowledgeable enough to know what the abbreviation means. So, I can change this if others (specifically @lucas-rami) also agree.

[pcineverdies]

Hi Rouzbeh!

Just for the sake of not taking decisions on our own, I'd like to ping @Jiahui17 and @murphe67 for what regards the appropriate name the attribute. Should it be something verbose such as active/inactive or something symbolic like A/I ?

[pcineverdies]

Now, here is my next concern.

As far as I could able to test, it seems that, with your modification, the functionalities related to the old minimizeLSQ pass are not working anymore. I'll consider video_filter, which is one of the examples in which all the LSQs should be removed due to data dependency between operations:

your branch:

• handshake.mlir:

  handshake.func @video_filter(%arg0: !handshake.channel<i32>, %arg1: !handshake.channel<i32>, %arg2: memref<900xi32>, %arg3: memref<900xi32>, %arg4: memref<900xi32>, %arg5: !handshake.control<>, %arg6: !handshake.control<>, %arg7: !handshake.control<>, %arg8: !handshake.control<>, ...) -> (!handshake.control<>, !handshake.control<>, !handshake.control<>, !handshake.control<>) attributes {argNames = ["offset", "scale", "pixelR", "pixelG", "pixelB", "pixelR_start", "pixelG_start", "pixelB_start", "start"], resNames = ["pixelR_end", "pixelG_end", "pixelB_end", "end"]} {
    %0:2 = lsq[%arg4 : memref<900xi32>] (%arg7, %result_0, %addressResult_4, %addressResult_6, %dataResult_7, %result_30)  {groupSizes = [2 : i32], handshake.name = "lsq0"} : (!handshake.control<>, !handshake.control<>, !handshake.channel<i32>, !handshake.channel<i32>, !handshake.channel<i32>, !handshake.control<>) -> (!handshake.channel<i32>, !handshake.control<>)
    %1:2 = lsq[%arg3 : memref<900xi32>] (%arg6, %result_0, %addressResult_8, %addressResult_10, %dataResult_11, %result_30)  {groupSizes = [2 : i32], handshake.name = "lsq1"} : (!handshake.control<>, !handshake.control<>, !handshake.channel<i32>, !handshake.channel<i32>, !handshake.channel<i32>, !handshake.control<>) -> (!handshake.channel<i32>, !handshake.control<>)
    %2:2 = lsq[%arg2 : memref<900xi32>] (%arg5, %result_0, %addressResult, %addressResult_2, %dataResult_3, %result_30)  {groupSizes = [2 : i32], handshake.name = "lsq2"} : (!handshake.control<>, !handshake.control<>, !handshake.channel<i32>, !handshake.channel<i32>, !handshake.channel<i32>, !handshake.control<>) -> (!handshake.channel<i32>, !handshake.control<>)
...

• handshake_transformed.mlir:

  handshake.func @video_filter(%arg0: !handshake.channel<i32>, %arg1: !handshake.channel<i32>, %arg2: memref<900xi32>, %arg3: memref<900xi32>, %arg4: memref<900xi32>, %arg5: !handshake.control<>, %arg6: !handshake.control<>, %arg7: !handshake.control<>, %arg8: !handshake.control<>, ...) -> (!handshake.control<>, !handshake.control<>, !handshake.control<>, !handshake.control<>) attributes {argNames = ["offset", "scale", "pixelR", "pixelG", "pixelB", "pixelR_start", "pixelG_start", "pixelB_start", "start"], resNames = ["pixelR_end", "pixelG_end", "pixelB_end", "end"]} {
    %0:3 = fork [3] %arg8 {handshake.bb = 0 : ui32, handshake.name = "fork0"} : <>
    %1:2 = lsq[%arg4 : memref<900xi32>] (%arg7, %44#2, %addressResult_4, %addressResult_6, %dataResult_7, %109#2)  {groupSizes = [2 : i32], handshake.name = "lsq3"} : (!handshake.control<>, !handshake.control<>, !handshake.channel<i10>, !handshake.channel<i10>, !handshake.channel<i32>, !handshake.control<>) -> (!handshake.channel<i32>, !handshake.control<>)
    %2:2 = lsq[%arg3 : memref<900xi32>] (%arg6, %44#1, %addressResult_8, %addressResult_10, %dataResult_11, %109#1)  {groupSizes = [2 : i32], handshake.name = "lsq4"} : (!handshake.control<>, !handshake.control<>, !handshake.channel<i10>, !handshake.channel<i10>, !handshake.channel<i32>, !handshake.control<>) -> (!handshake.channel<i32>, !handshake.control<>)
    %3:2 = lsq[%arg2 : memref<900xi32>] (%arg5, %44#0, %addressResult, %addressResult_2, %dataResult_3, %109#0)  {groupSizes = [2 : i32], handshake.name = "lsq5"} : (!handshake.control<>, !handshake.control<>, !handshake.channel<i10>, !handshake.channel<i10>, !handshake.channel<i32>, !handshake.control<>) -> (!handshake.channel<i32>, !handshake.control<>)
...

As you can see, while the LSQs are supposed to be removed, they are all still there (see the original paper for a confirmation).
Clearly, this does not affect functionalities, but performance only, thus the tests were still passing!

However, what I have also noticed is that currently in the main branch the LSQs are not removed either. In particular, this happens since commit b66f343a0cc8f04468161a245a55bc2eced4729c.

I will let you debug the code on your own. I just have a couple of doubts, which maybe might help you when dealing with the code:

1. In the handshake_transfored.mlir that you produce, the dependencies are still marked as active, while they should not.
2. Is it legit that you have modified the ex-minimizeLSQ but not replaceMemInterface pass? To me these two passes are strongly dependent, since the information of the former is used by the latter; if you modify the "communication protocol" of the first, why the second should be uneffected? Maybe you have a clever answer to my ignorance here!
3. During an offline conversation, you told me that the modifications you have introduce were in conflict with the unit tests to the handshake dialect (run via ninja). I see no modifications in this sense in the PR though.

[rpirayadi]

Thanks for your comments!

However, what I have also noticed is that currently in the main branch the LSQs are not removed either. In particular, this happens since commit b66f343.

The point this happens is not b66f343, but It is 13948ba. What's happening is that previously no path was added to the paths in here. So, in here the results was always true. And, previously the output of isStoreGIIDOnLoad was always true! That means previously AnalyzeLSQUsage pass was removing LSQs more than it should have!

The problem is only for this specific example which is using shift operation. That's because some operations are missing here. The one that is causing the problem in this case is handshake::ShRSIOp. But, I've also added other ones that were missing.

Is it legit that you have modified the ex-minimizeLSQ but not replaceMemInterface pass? To me these two passes are strongly dependent, since the information of the former is used by the latter; if you modify the "communication protocol" of the first, why the second should be uneffected? Maybe you have a clever answer to my ignorance here!

About this the communication protocol is unchanged, since I'm still marking the LSQ ports.

During an offline conversation, you told me that the modifications you have introduce were in conflict with the unit tests to the handshake dialect (run via ninja). I see no modifications in this sense in the PR though.

I managed to solve this by using ::mlir::BoolAttr.

[pcineverdies]

Thanks Rouzbeh, and yeah sorry, I was referring exactly to that commit (then when adding the link I was confused by you being the author). I also get your answers to the other two points.

I see now that video_filter works as expected.

Could you please have a look at threshold as well? According to the paper, this example should result in no LSQs; this also makes sense to me, since the store operations cannot run before the load operations are done. Again, before 13948ba this was accomplished, while now it is not.

P.S. I'm pointing out these issues because it makes little sense to me to work on a system which does not do what it's supposed to do in the first place. Considering also that your work and all our benchmarks are affected by this.

[rpirayadi]

There was a very strange bug here which was causing this!
The comment itself says or but and is used in the code!!!

[murphe67]

From a very brief skim:

On a language level, inactivate is not a word. English pairs the verb "deactivate" with the adjective "inactive". This is a split second confusion for me when I read it if you mean enact or deactivate, but I'm not sure this is a big deal.

Similarly, it looks like you've mixed up insure and ensure in one of the comments.

On the actual topic you pinged me on, for me it's a question of redundancy: if the fact that this is a memory dependency status is always clear, then A/I is fine. If there are other categories that are relevant to memory dependency (inter-loop vs. intra-loop, 2-iteration-distance dependency vs. compile-time unknown index, etc.), that could cause confusion, active/inactive would be better.

[pcineverdies]

I am glad to see (for a project perspective) that the CI/CD is working out as expected, stating that the test lu is failing (@paolo-ienne @ebosnjak).
Whenever you have time to address the failing test in here, I'd love to know what happened :)

[paolo-ienne]

😄