Add InterBlock loop/epilogue analysis #51

andcarminati · 2024-05-29T10:09:38Z

Implement loop/epilogue analysis to reduce pessimism related to the propagation of loop latencies and resource conflicts to epilogue.
This PR adds:

MemoryEdges update to ignore bundles.
Latency analysis using a DDG.
Conflict analysis using two scoreboards.
Code refactoring.
Test additions/updates.

andcarminati · 2024-05-29T11:46:01Z

Benchmark evaluation:

With option --aie-loop-min-tripcount=4

First results are without the change and the second are with.

+----------------+-------------------+------------------+--+-------------------+------------------+--+--------------------+-------------------+
|     Design     | Total Cycle Count | Total Insn Count |  | Total Cycle Count | Total Insn Count |  | % Dec. Cycle Count | % Dec. Insn Count |
+----------------+-------------------+------------------+--+-------------------+------------------+--+--------------------+-------------------+
| GEMM_bf16_0    |             10994 |             4670 |  |             10898 |             4574 |  | 0,87%              | 2,06%             |
| GEMV_0         |              3163 |              689 |  |              3163 |              689 |  | 0,00%              | 0,00%             |
| Mul2D_0        |              4807 |             1643 |  |              4807 |             1643 |  | 0,00%              | 0,00%             |
| AvgPool2D_0    |              5153 |             3299 |  |              5153 |             3299 |  | 0,00%              | 0,00%             |
| Pad2D_0        |              4522 |             2190 |  |              4522 |             2190 |  | 0,00%              | 0,00%             |
| MaxPool2D_0    |              3832 |             2483 |  |              3832 |             2483 |  | 0,00%              | 0,00%             |
| GEMV_1         |              1969 |              587 |  |              1969 |              587 |  | 0,00%              | 0,00%             |
| Add2D_0        |              8157 |             3478 |  |              8157 |             3478 |  | 0,00%              | 0,00%             |
| Conv2D_0       |             20649 |            10943 |  |             20239 |            10533 |  | 1,99%              | 3,75%             |
| Conv2D_ReLU_0  |              3391 |             1716 |  |              3331 |             1656 |  | 1,77%              | 3,50%             |
| Conv2D_LReLU_0 |              4768 |             3094 |  |              4708 |             3034 |  | 1,26%              | 1,94%             |
+----------------+-------------------+------------------+--+-------------------+------------------+--+--------------------+-------------------+

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp

martien-de-jong

Nothing wrong. Just a few nits you may want to consider.

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp

gbossu · 2024-06-03T08:58:54Z

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp

+  AddRegionToEdges(LoopBS.getBottom());
+  Edges.markBoundary();
+  // Second part is the epilogue itself
+  AddRegionToEdges(EpilogueBS.getTop());


Nit: Maybe also add the ExitSU node of EpilogueBS to the map of distances so we can avoid the if (Succ->isBoundaryNode()) corner case in the loop below.

In the current implementation, this mapping maps instructions to depths, in this way we need to change the mapping logic as well.

What I meant is something like: DistancesFromLoopEntry[EpilogueBS.getTop().getExitSU()] = DistFromLoopEntry;, this way there is no if (Succ->isBoundaryNode()) special casing in the loop below.

Hi, I simplified a bit more the code.

gbossu

This looks good, I think the last remaining thing would be to fix the assertion that now triggers in the RegionEndEdges mutator for some benchmarks, and add a small reproducer test for it.

On top of that, please look at the Nit comments and address up to taste :) Generally, I'd be happy if we can get rid of the if (Succ->isBoundaryNode()) special casing.

andcarminati · 2024-06-04T09:51:50Z

Hi @martien-de-jong and @gbossu, I addressed all the comments and suggestions. Benchmarks are running again with no problems. I you can take a look, it would be nice.

This change is necessary to run correctly mutations after bundling.

andcarminati · 2024-06-06T09:24:33Z

Here the most recent results:


+----------------+---+------------+---------------+-------------+---+---+------------+---------------+-------------+---+---+-------------------+----------------+--------------------+
|       #        | # | Without EA |       #       |      #      | # | # |  With EA   |       #       |      #      | # | # |       Comp.       |       #        |         #          |
+----------------+---+------------+---------------+-------------+---+---+------------+---------------+-------------+---+---+-------------------+----------------+--------------------+
| Design         |   | Tot. PM S. | Tot. Cycle C. | Tot. Ins C. |   |   | Tot. PM S. | Tot. Cycle C. | Tot. Ins C. |   |   | % Diff Tot. PM S. | % Diff Tot. C. | % Diff Tot. Ins C. |
| GEMM_bf16_0    |   | 4160       | 10461         | 4160        |   |   | 4144       | 10365         | 4064        |   |   | -0,38%            | -0,92%         | -2,31%             |
| GEMV_0         |   | 3488       | 3126          | 650         |   |   | 3488       | 3126          | 650         |   |   | 0,00%             | 0,00%          | 0,00%              |
| Mul2D_0        |   | 2672       | 4857          | 1686        |   |   | 2672       | 4857          | 1686        |   |   | 0,00%             | 0,00%          | 0,00%              |
| AvgPool2D_0    |   | 3552       | 4159          | 2811        |   |   | 3552       | 4159          | 2811        |   |   | 0,00%             | 0,00%          | 0,00%              |
| Pad2D_0        |   | 3456       | 4512          | 2184        |   |   | 3456       | 4512          | 2184        |   |   | 0,00%             | 0,00%          | 0,00%              |
| MaxPool2D_0    |   | 3344       | 3798          | 2449        |   |   | 3344       | 3798          | 2449        |   |   | 0,00%             | 0,00%          | 0,00%              |
| GEMV_1         |   | 3488       | 1948          | 560         |   |   | 3488       | 1948          | 560         |   |   | 0,00%             | 0,00%          | 0,00%              |
| Add2D_0        |   | 3600       | 8157          | 3478        |   |   | 3600       | 8157          | 3478        |   |   | 0,00%             | 0,00%          | 0,00%              |
| Conv2D_0       |   | 5952       | 19018         | 10543       |   |   | 5920       | 18538         | 10063       |   |   | -0,54%            | -2,52%         | -4,55%             |
| Conv2D_ReLU_0  |   | 5328       | 3377          | 1704        |   |   | 5296       | 3317          | 1644        |   |   | -0,60%            | -1,78%         | -3,52%             |
| Conv2D_LReLU_0 |   | 6000       | 4767          | 3095        |   |   | 5968       | 4707          | 3035        |   |   | -0,53%            | -1,26%         | -1,94%             |
+----------------+---+------------+---------------+-------------+---+---+------------+---------------+-------------+---+---+-------------------+----------------+--------------------+

gbossu · 2024-06-06T15:54:30Z

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp

+  while (Bottom.conflict(Top, Depth)) {
+    Bottom.advance();
+    NopCounter++;
+  }


Nit: That looks a lot like AIEPostRASchedStrategy::handleRegionConflicts, maybe there's code we can share.

Hi @gbossu, I think it could be a bit complicated, because handleRegionConflicts uses two hazard recognizers, while we use just one, plus scoreboard comparison. I am afraid that a refactor could create a more confusing code.

The implementation is different, but ultimately the goal is the same, we want to insert NOPs until there is no resource hazard. We can do that in a follow-up PR.

gbossu

LGTM, I think it's great work!

Nit: I'd be even happier if we could remove the if (Succ->isBoundaryNode()) special case in getCyclesToRespectTiming(), maybe by extending the map of depths, maybe by using a conditional assignment instead.

andcarminati · 2024-06-10T15:08:43Z

LGTM, I think it's great work!

Nit: I'd be even happier if we could remove the if (Succ->isBoundaryNode()) special case in getCyclesToRespectTiming(), maybe by extending the map of depths, maybe by using a conditional assignment instead.

Thank you for this suggestion, I addressed this special case.

gbossu

LGTM

gbossu · 2024-06-10T15:35:22Z

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp

+          continue;
+        const MachineInstr *PostBoundaryMI = Succ->getInstr();
+
+        int PostBondOrExitDist =


Nit PostBoundOrExitDist

gbossu · 2024-06-10T15:43:16Z

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp

+  while (Bottom.conflict(Top, Depth)) {
+    Bottom.advance();
+    NopCounter++;
+  }


The implementation is different, but ultimately the goal is the same, we want to insert NOPs until there is no resource hazard. We can do that in a follow-up PR.

Implement loop/epilog analysis to reduce pessimism related to the propagation of loop latencies and resource conflicts to the epilogue. This commit adds: * Latency analysis using a DDG. * Conflict analysis using two scoreboards. * Code refactoring. * Test additions/updates.

Now, this mutation can be applied to already bundled instructions without misclassification. It means that it is safe to run this mutation with Epilogue scheduling, for example.

If cast is necessary, MaxLatencyFinder will do it anyway.

martien-de-jong

LGTM

andcarminati requested review from gbossu and martien-de-jong May 29, 2024 10:09

martien-de-jong reviewed May 31, 2024

View reviewed changes

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp Outdated Show resolved Hide resolved

martien-de-jong reviewed May 31, 2024

View reviewed changes

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp Outdated Show resolved Hide resolved

martien-de-jong reviewed May 31, 2024

View reviewed changes

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp Show resolved Hide resolved

martien-de-jong previously approved these changes May 31, 2024

View reviewed changes

andcarminati force-pushed the andreu.me.loop.epilogue branch from 62899cc to d74b615 Compare May 31, 2024 16:19

gbossu reviewed Jun 3, 2024

View reviewed changes

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp Outdated Show resolved Hide resolved

gbossu reviewed Jun 3, 2024

View reviewed changes

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp Outdated Show resolved Hide resolved

gbossu reviewed Jun 3, 2024

View reviewed changes

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp Outdated Show resolved Hide resolved

gbossu reviewed Jun 3, 2024

View reviewed changes

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp Outdated Show resolved Hide resolved

gbossu reviewed Jun 3, 2024

View reviewed changes

andcarminati force-pushed the andreu.me.loop.epilogue branch 2 times, most recently from 0f3a798 to 7c6d248 Compare June 4, 2024 09:32

andcarminati dismissed martien-de-jong’s stale review via 7c6d248 June 4, 2024 21:53

andcarminati force-pushed the andreu.me.loop.epilogue branch from 7c6d248 to 038a23a Compare June 5, 2024 08:19

[AIE2] Change MemoryEdges to ignore bundles

da2cc67

This change is necessary to run correctly mutations after bundling.

andcarminati force-pushed the andreu.me.loop.epilogue branch from 038a23a to 50d5ed6 Compare June 6, 2024 06:51

andcarminati requested review from abhinay-anubola, abnikant, khallouh, konstantinschwarz, SagarMaheshwari99 and stephenneuendorffer as code owners June 6, 2024 06:51

gbossu reviewed Jun 6, 2024

View reviewed changes

gbossu previously approved these changes Jun 6, 2024

View reviewed changes

andcarminati dismissed gbossu’s stale review via e42ec66 June 10, 2024 15:05

andcarminati force-pushed the andreu.me.loop.epilogue branch from 50d5ed6 to e42ec66 Compare June 10, 2024 15:05

gbossu previously approved these changes Jun 10, 2024

View reviewed changes

andcarminati dismissed gbossu’s stale review via ab33106 June 11, 2024 08:55

andcarminati force-pushed the andreu.me.loop.epilogue branch from e42ec66 to ab33106 Compare June 11, 2024 08:55

gbossu approved these changes Jun 11, 2024

View reviewed changes

andcarminati added 3 commits June 11, 2024 10:07

[AIE2] Fix RegionEndEdges for bundled Instructions

bb11e1e

Now, this mutation can be applied to already bundled instructions without misclassification. It means that it is safe to run this mutation with Epilogue scheduling, for example.

[AIE2] Remove unnecessary cast to AIEScheduleDAGMI

831ac27

If cast is necessary, MaxLatencyFinder will do it anyway.

andcarminati force-pushed the andreu.me.loop.epilogue branch from ab33106 to 831ac27 Compare June 11, 2024 09:09

gbossu approved these changes Jun 11, 2024

View reviewed changes

martien-de-jong approved these changes Jun 11, 2024

View reviewed changes

andcarminati merged commit ebbc1af into aie-public Jun 11, 2024
8 checks passed

gbossu deleted the andreu.me.loop.epilogue branch June 14, 2024 08:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add InterBlock loop/epilogue analysis #51

Add InterBlock loop/epilogue analysis #51

andcarminati commented May 29, 2024

andcarminati commented May 29, 2024

martien-de-jong left a comment

gbossu Jun 3, 2024

andcarminati Jun 4, 2024

gbossu Jun 6, 2024

andcarminati Jun 10, 2024

gbossu left a comment •

edited

Loading

andcarminati commented Jun 4, 2024

andcarminati commented Jun 6, 2024

gbossu Jun 6, 2024

andcarminati Jun 10, 2024

gbossu Jun 10, 2024

gbossu left a comment

andcarminati commented Jun 10, 2024

gbossu left a comment

gbossu Jun 10, 2024

gbossu Jun 10, 2024

martien-de-jong left a comment

Add InterBlock loop/epilogue analysis #51

Add InterBlock loop/epilogue analysis #51

Conversation

andcarminati commented May 29, 2024

andcarminati commented May 29, 2024

martien-de-jong left a 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

gbossu left a comment • edited Loading

Choose a reason for hiding this comment

andcarminati commented Jun 4, 2024

andcarminati commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbossu left a comment

Choose a reason for hiding this comment

andcarminati commented Jun 10, 2024

gbossu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martien-de-jong left a comment

Choose a reason for hiding this comment

gbossu left a comment •

edited

Loading