New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[AIE NFC] Postpipeliner cleanups and refactorings #253

Open

martien-de-jong wants to merge 9 commits into aie-public from martien.smallfixes

+408 −66

Collaborator

martien-de-jong commented Jan 8, 2025

Small logging improvements
Facility to rerun a strategy an arbitrary number of times
Push earliest with 'local' resource conflicts

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

January 8, 2025 14:39

martien-de-jong force-pushed the martien.smallfixes branch from 2f11c48 to 3e5d4c7 Compare

January 9, 2025 12:22

martien-de-jong commented

View reviewed changes

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp

+              static cl::opt<int> PostPipelinerMaxTryII(
+                  "aie-postpipeliner-maxtry-ii", cl::init(10),
+                  cl::desc("[AIE] Maximum II steps to be tried in the post-ra pipeliner"));

Collaborator Author

martien-de-jong Jan 8, 2025

Rationale: It is difficult to give a useful absolute value. for small ResMII we will be trying way beyond usefulness, for larger ResMII it may not be enough. A relative amount works better.

martien-de-jong commented

View reviewed changes

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp

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

Collaborator Author

martien-de-jong Jan 10, 2025

This also simplifies using a solver only on the first few tries

andcarminati reviewed

View reviewed changes

llvm/lib/Target/AIE/AIEPostPipeliner.h

@@ @@ -139,9 +161,10 @@ class PostPipeliner { @@
                 int FirstUnscheduled = 0;
                 int LastUnscheduled = -1;
-                /// Holds the cycle of each SUnit. The following should hold:
+                /// Holds the schuling information for each instruction. The following

Collaborator

andcarminati Jan 10, 2025

schuling -> scheduling

andcarminati reviewed

View reviewed changes

llvm/lib/Target/AIE/AIEPostPipeliner.h

+              public:
+                std::vector<NodeInfo> Nodes;
+                int NInstr;
+                int MaxEarliest = 0;

Collaborator

andcarminati Jan 10, 2025

It would be nice to have some comments on these fields, for future reference.

Collaborator

andcarminati Jan 10, 2025

Check: those fields are not used now. Are they related to a new feature?

Collaborator Author

martien-de-jong Jan 14, 2025

ScheduleInfo is passed to the strategies. These values are computed as a service to implementors of new strategies.

andcarminati reviewed

View reviewed changes

llvm/test/CodeGen/AIE/aie2/schedule/postpipeliner/conv2d_bf16-feasibleRA.mir Outdated

+              # See https://llvm.org/LICENSE.txt for license information.
+              # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+              #
+              # (c) Copyright 2024 Advanced Micro Devices, Inc. or its affiliates

Collaborator

andcarminati Jan 10, 2025

nit: 2025.

andcarminati reviewed

View reviewed changes

llvm/test/CodeGen/AIE/aie2/schedule/postpipeliner/conv2d_bf16-feasibleRA.mir

+              #
+              # (c) Copyright 2024 Advanced Micro Devices, Inc. or its affiliates
+              # RUN: llc --mtriple=aie2 -O2 --start-before=postmisched %s \

Collaborator

andcarminati Jan 10, 2025

Can we have a description of what does feasibleRA means here? I can see that we have some conf2d_bf16 so it is interesting to also see how this one differs from the others.

Collaborator

andcarminati commented Jan 10, 2025

Hi @martien-de-jong, this set of changes looks good! I just left some comments for clarification!

Martien de Jong added 9 commits

January 14, 2025 15:14


          [AIE] Pack NodeInfo in a class with additional information

8baa1cf

This is to give full access to the Info array and it's associated parameters


          [AIE] Push Earliest with local resource contention

03951f0

Dump intervals in ascii art


          [AIE] Specify maxii relative to MinII

3cd1db1


          [AIE] Compute MinEarliest, MaxEarliest and MinLatest

37d1a95


          [AIE] Don't print edges to ExitSU

505d73f


          [AIE] Maintain II in the pipeliner strategy

11bc08e


          [AIE] Add conv2d-bf16 test with feasible register allocation

6636a0a


          [AIE] Bail out if RecMII not met.

7f4c316


          [AIE] Reject NStages=1 pipelines

95dbddf

martien-de-jong force-pushed the martien.smallfixes branch from 3e5d4c7 to 95dbddf Compare

January 14, 2025 14:41

gbossu reviewed

View reviewed changes

llvm/lib/Target/AIE/AIEPostPipeliner.cpp

+                  bool TopDown = true;
+                  bool Alternate = false;
+                  int Runs = 0;
+                  ArrayRef<PriorityComponent> Components;

Collaborator

gbossu Jan 29, 2025

Why not own the list of components? Does that help a lot with memory usage?

gbossu reviewed

View reviewed changes

llvm/lib/Target/AIE/AIEPostPipeliner.cpp

+                struct Configuration {
+                  int ExtraStages = 0;
+                  bool TopDown = true;
+                  bool Alternate = false;

Collaborator

gbossu Jan 29, 2025

Would be nice to document those fields, especially Alternate

gbossu reviewed

View reviewed changes

llvm/lib/Target/AIE/AIEPostPipeliner.cpp

+                  // When we need more slots than we have data predecessors, we have local
+                  // resource contention that we can safely account for in Earliest.
+                  if (Count > 0 && Slots.max() > Count) {
+                    Me.Earliest = std::max(Me.Earliest, PredEarliest + Slots.max() - 1);

Collaborator

gbossu Jan 29, 2025

Checking: Do we have something similar to bias Latest based on successors? I think yes, but my memory is blurry.

gbossu reviewed

View reviewed changes

llvm/lib/Target/AIE/AIEPostPipeliner.cpp

@@ @@ -342,7 +342,10 @@ bool PostPipeliner::computeLoopCarriedParameters() { @@
                   N.StaticLatest = N.Latest;
                 }
                 Info.compute();
-                return true;
+                // If no node can be scheduled in cycle 0, we must have a circuit that

Collaborator

gbossu Jan 29, 2025

Nit: If no node can be scheduled in cycle 0 after accounting for LCDs, ...

gbossu reviewed

View reviewed changes

llvm/lib/Target/AIE/AIEPostPipeliner.cpp

+                // represent a valid 'regular' loop schedule.
+                if (NStages == 1) {
+                  LLVM_DEBUG(dbgs() << "PostPipeliner: Degenerate pipeline, NStages=1\n");
+                  return false;

Collaborator

gbossu Jan 29, 2025

FYI @andcarminati , I think that will simplify the epilogue scheduling PR.

gbossu reviewed

View reviewed changes

Collaborator

gbossu left a comment

This looks good, and I think I want it. Let me know when you went through the comments and I will approve :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

gbossu gbossu left review comments

andcarminati andcarminati left review comments

abhinay-anubola Awaiting requested review from abhinay-anubola abhinay-anubola is a code owner

abnikant Awaiting requested review from abnikant abnikant is a code owner

khallouh Awaiting requested review from khallouh khallouh is a code owner

konstantinschwarz Awaiting requested review from konstantinschwarz konstantinschwarz is a code owner

SagarMaheshwari99 Awaiting requested review from SagarMaheshwari99 SagarMaheshwari99 is a code owner

stephenneuendorffer Awaiting requested review from stephenneuendorffer stephenneuendorffer is a code owner

At least 1 approving review is required to merge this pull request.

Labels

None yet