Martien.pre2post #213

martien-de-jong · 2024-10-15T15:43:08Z

Some tweaking to get more leverage from the PostPipeliner:

Pre-allocate resources of future operations to take more heed of possible conflicts
Avoid unrolling for very high MinTripCount loops
PrePipeliner doesn't increase II if MaxStages is exceeded, instead it stops searching and rejects the schedule in favour of pipelining
Allow larger II's in PostPipeliner; compute ResMII to take away some compile time

gbossu · 2024-10-16T08:43:45Z

llvm/lib/Target/AIE/AIEPostPipeliner.cpp

+      HR.emitInScoreboard(Scoreboard, MI->getDesc(), MemoryBanks,
+                          MI->operands(), MI->getMF()->getRegInfo(), Cycle);
+      Cycle += II;
+    }


Could you think of maybe adding a small test that benefits from "pre-booking modulo resources"?

I'll see whether this tipped the conv2d balamce.

gbossu · 2024-10-16T08:45:17Z

llvm/lib/CodeGen/MachinePipeliner.cpp

@@ -141,7 +141,7 @@ static cl::opt<int> SwpForceII("pipeliner-force-ii",
 static cl::opt<int>
    SwpMaxStages("pipeliner-max-stages",
                 cl::desc("Maximum stages allowed in the generated scheduled."),
-                 cl::Hidden, cl::init(3));
+                 cl::Hidden, cl::init(-1));


We might have trouble upstreaming that

In the end, this option seems strange. This decision should always delegated to the target.

On the other hand, we can push this option to -1 from the frontend.

I guess this predates shouldUseSchedule. I agree with Andreu that it should be decided there.
When we upstream, I'll repair machine-pipeliner users, e.g. by implementing it in the base class of PipelinerLoopInfo rather than in the pipeliner itself.

In fact I will do that in this PR.

Hmm. It's more complicated than that, because there's a MaxStages test in the main II loop. That means it could be moved to canAcceptII, but that's not upstreamed either. I propose to upstream both at the same time, moving the MaxStages check to the default implementation of canAcceptII

llvm/lib/Target/AIE/AIEMachineScheduler.cpp

andcarminati · 2024-10-17T09:54:16Z

llvm/include/llvm/CodeGen/ScheduleDAGInstrs.h

@@ -272,6 +272,9 @@ namespace llvm {
    MachineFunction *CurrFunc;
    MachineFunction::iterator NextMBB;

+    // Supply specific memory disambiguation


I like this flexibility!

martien-de-jong · 2024-10-17T11:30:54Z

llvm/lib/Target/AIE/AIEBasePipelinerLoopInfo.cpp

+  // PostPipeliner can do nothing without tripcount > 1
+  if (MinTripCount <= 1) {
+    return false;
+  }


Hmm. It's a ZOL, Prepipeliner can't do anything either.

Why not? We can add guards I think

I feel that is hard to add guards in post, it will change the CFG and will require a new register for the constant...

Without a loop counter GPR?

martien-de-jong · 2024-10-17T11:32:30Z

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp

@@ -55,7 +55,7 @@ static cl::opt<bool>
                       "iterative loop scheduling"));

 static cl::opt<int> PostPipelinerMaxII(
-    "aie-postpipeliner-maxii", cl::init(10),
+    "aie-postpipeliner-maxii", cl::init(40),


Probably too high. Maybe revisit when we hit noticable compile time.

It's okay, if makes me feel better about "aie-loop-aware-expensive-iterations", cl::init(35) 😆

martien-de-jong · 2024-10-17T11:35:46Z

llvm/test/CodeGen/AIE/aie2/schedule/swp/swp-zoloop-accept-reject.mir

-  ; CHECK-TC-2: PLI: Accepting II=5
-
-  ; CHECK-TC-1: Not interesting MinTripCount (<=1)!
-  ; CHECK-TC-1-NEXT: Loop rejected


Questionable auto-update.

Yes I think the # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 4 header should be removed. Could you maybe manually update the test?

martien-de-jong · 2024-10-17T11:47:40Z

llvm/lib/Target/AIE/AIEHazardRecognizer.cpp

+        AddrSpace                ? ASI.getMemoryBanksFromAddressSpace(AddrSpace)
+        : AddressSpaceNoneIsSafe ? 0
+                                 : ASI.getDefaultMemoryBank();
+    MemoryBankUsed |= MemoryBank;


Note that this is more than an added option. We start at none, and collect the memory banks from all MMO's. We used to start at 'all banks' and intersect. The latter is incompatible with multi-memory ops like VLDB.4x

martien-de-jong · 2024-10-17T11:48:59Z

llvm/lib/Target/AIE/AIE2TargetTransformInfo.cpp

@@ -54,6 +54,9 @@ static cl::opt<int>
 static cl::opt<unsigned>
    MaxUnrollCost("aie-unroll-max-cost", cl::Hidden, cl::init(200),
                  cl::desc("Maximum partial unroll cost for loops"));
+static cl::opt<unsigned> PreferSwpOverUnroll(
+    "aie-prefer-swp-over-unroll", cl::Hidden, cl::init(9),
+    cl::desc("Aim for pipelining if MinIterCount is at least this value."));


The default is inspired by the fact that all ReduceABC's regress for 8 and lower.

martien-de-jong · 2024-10-17T11:54:01Z

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp

+  if (BS.getRegions().size() == 1) {
+    auto &PostSWP = BS.getPostSWP();
+    if (PostSWP.canAccept(*BS.TheBlock)) {
+      BS.FixPoint.II = PostSWP.getResMII(*BS.TheBlock);


This saves a lot of work. II divides the linear schedule length to determine the number of copies in the DAG, and DAG construction is expensive and worse than linear in the number of nodes. Something like Conv2D immediately hits the target II, so we would also save 7 iterations of the main loop.

llvm/lib/Target/AIE/AIEPostPipeliner.cpp

llvm/lib/Target/AIE/AIEBasePipelinerLoopInfo.cpp

llvm/lib/Target/AIE/AIEPostPipeliner.cpp

andcarminati · 2024-10-18T08:53:54Z

llvm/lib/Target/AIE/AIEPostPipeliner.cpp

+    while (C < NInstr && (Scoreboard[C] & Slots)) {
+      C++;
+    }
+    if (C >= NInstr) {


nit: C == NInstr. with C > NInstr means that we need k+1 slots for k instructions.

nitter: This is the reverse of the first part of the loop condition, without having to know how C is updated.

andcarminati · 2024-10-18T09:00:24Z

llvm/lib/Target/AIE/AIEPostPipeliner.cpp

+      C++;
+    }
+    if (C >= NInstr) {
+      MII = NInstr;


When this happens, do we try to run the pipeliner anyway? Should we return PostPipelinerMaxII (provided we share this option)?

This computes ResMII, not how it it used. I don't want to tie too many strings to it.
As we compute it now, it will never exceed NInstr. but even if II == NInstr, the latency path through those NInstrs can allow a 4 stage pipeline to be useful. And if the DAG is a straight chain of dependences, pipelining can still be cheap.

andcarminati · 2024-10-18T09:03:03Z

llvm/lib/Target/AIE/AIEPostPipeliner.cpp

@@ -270,8 +300,15 @@ bool PostPipeliner::scheduleFirstIteration() {
    LLVM_DEBUG(dbgs() << "  Emit in " << -Depth + LocalCycle << "\n");
    int Cycle = -Depth + LocalCycle;
    LLVM_DEBUG(dbgs() << "  Emit in " << Cycle << "\n");
-    HR.emitInScoreboard(Scoreboard, MI->getDesc(), MemoryBanks, MI->operands(),
-                        MI->getMF()->getRegInfo(), Cycle);
+    for (int N = 0; N < NCopies; N++) {


nit: I would use unsigned here ;-).

I'm sorry, I solemnly pledged only to use unsigned for bitmasks (and perhaps for very small types like char for lookup tables)

gbossu · 2024-10-21T10:12:02Z

llvm/lib/Target/AIE/AIEPostPipeliner.cpp

+        if (NewLatest < Info[P].Latest) {
+          Info[P].Latest = NewLatest;
+          Changed = true;
+        }


Can you give an overview of what this commit is doing?

llvm/test/CodeGen/AIE/aie2/schedule/postpipeliner/conv2d.mir

gbossu

LGTM 🎉

…heduling the first

Also tweak II hand-off limit

Targets can cater for this in shouldUseSchedule()

martien-de-jong requested review from abhinay-anubola, abnikant, andcarminati, gbossu, khallouh, konstantinschwarz, SagarMaheshwari99 and stephenneuendorffer as code owners October 15, 2024 15:43

martien-de-jong marked this pull request as draft October 15, 2024 15:43

martien-de-jong force-pushed the martien.pre2post branch from f08b580 to 8028243 Compare October 15, 2024 16:15

gbossu reviewed Oct 16, 2024

View reviewed changes

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

andcarminati reviewed Oct 17, 2024

View reviewed changes

martien-de-jong force-pushed the martien.pre2post branch from 8028243 to c619de9 Compare October 17, 2024 11:09

martien-de-jong marked this pull request as ready for review October 17, 2024 11:10

martien-de-jong commented Oct 17, 2024

View reviewed changes

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

gbossu reviewed Oct 17, 2024

View reviewed changes

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

gbossu reviewed Oct 17, 2024

View reviewed changes

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

martien-de-jong force-pushed the martien.pre2post branch from c619de9 to 6a433ee Compare October 17, 2024 14:39

andcarminati reviewed Oct 18, 2024

View reviewed changes

martien-de-jong force-pushed the martien.pre2post branch from 6a433ee to e8dfeb1 Compare October 18, 2024 13:44

gbossu reviewed Oct 21, 2024

View reviewed changes

gbossu reviewed Oct 22, 2024

View reviewed changes

llvm/test/CodeGen/AIE/aie2/schedule/postpipeliner/conv2d.mir Show resolved Hide resolved

martien-de-jong force-pushed the martien.pre2post branch from e8dfeb1 to 3eaff9a Compare October 22, 2024 12:03

gbossu approved these changes Oct 22, 2024

View reviewed changes

Martien de Jong added 11 commits October 22, 2024 15:21

[AIE] Rename typo in test name

ba7eb3f

[AIE] add reg-renamed conv2d and hardsigmoid_templated example

6781f7a

[AIE-POSTPIPELINER ] pre-reserve resources of all iterations while sc…

b97f56d

…heduling the first

[AIE] ZOLoop leaves high stage count to postpipeliner

8058efa

Also tweak II hand-off limit

[MachinePipeliner] No default SwpMaxStages

971e24d

Targets can cater for this in shouldUseSchedule()

[SCHEDULEDAGINSTR] AliasAnalysis hook

0db4f18

[AIE] Use alias hook for virtual unroll.

3bebecd

[AIE] option to quench bank conflicts on addrspace(0) pointers

f606c02

[AIE] Don't unroll for pipelining candidates (as per MinIterCount)

11b3b5e

[AIE PostPipeliner] compute and use ResMII for a pipeline candidate loop

f9309d2

[AIE-POSTPIPELINER] More compact logging

061a3ae

martien-de-jong force-pushed the martien.pre2post branch from 3eaff9a to 061a3ae Compare October 22, 2024 13:21

martien-de-jong enabled auto-merge (rebase) October 22, 2024 13:22

martien-de-jong merged commit 9fcbc8e into aie-public Oct 22, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Martien.pre2post #213

Martien.pre2post #213

martien-de-jong commented Oct 15, 2024

gbossu Oct 16, 2024

gbossu Oct 17, 2024

martien-de-jong Oct 18, 2024

gbossu Oct 16, 2024

andcarminati Oct 17, 2024

andcarminati Oct 17, 2024

martien-de-jong Oct 17, 2024 •

edited

Loading

martien-de-jong Oct 18, 2024

martien-de-jong Oct 18, 2024 •

edited

Loading

andcarminati Oct 17, 2024

martien-de-jong Oct 17, 2024

gbossu Oct 17, 2024

andcarminati Oct 18, 2024

martien-de-jong Oct 18, 2024

martien-de-jong Oct 17, 2024

gbossu Oct 17, 2024

martien-de-jong Oct 17, 2024

gbossu Oct 17, 2024

martien-de-jong Oct 17, 2024

martien-de-jong Oct 17, 2024

martien-de-jong Oct 17, 2024

andcarminati Oct 18, 2024

martien-de-jong Oct 18, 2024

andcarminati Oct 18, 2024 •

edited

Loading

martien-de-jong Oct 18, 2024

andcarminati Oct 18, 2024

martien-de-jong Oct 18, 2024 •

edited

Loading

gbossu Oct 21, 2024

gbossu left a comment

Martien.pre2post #213

Martien.pre2post #213

Conversation

martien-de-jong commented Oct 15, 2024

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

martien-de-jong Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martien-de-jong Oct 18, 2024 • 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

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

andcarminati Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martien-de-jong Oct 18, 2024 • edited Loading

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

martien-de-jong Oct 17, 2024 •

edited

Loading

martien-de-jong Oct 18, 2024 •

edited

Loading

andcarminati Oct 18, 2024 •

edited

Loading

martien-de-jong Oct 18, 2024 •

edited

Loading