Skip to content
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

[AIEX] Actively avoid bank conflicts in the scheduler #95

Merged
merged 5 commits into from
Aug 14, 2024
Merged

[AIEX] Actively avoid bank conflicts in the scheduler #95

merged 5 commits into from
Aug 14, 2024

Conversation

krishnamtibrewala
Copy link
Collaborator

@krishnamtibrewala krishnamtibrewala commented Jun 25, 2024 •
edited
Loading

Use bank annotation information in Hazard recognizer to avoid scheduling multiple memory operation to a bank in same cycle.

In the current approach

  1. Two VLD will only be issued together if we have bank annotation for one or both.

@krishnamtibrewala krishnamtibrewala force-pushed the aie-avoid-bank-conflicts branch 7 times, most recently from 4203db5 to f92675a Compare June 26, 2024 15:10
@krishnamtibrewala krishnamtibrewala force-pushed the aie-avoid-bank-conflicts branch 2 times, most recently from 33f4c80 to 5fd0102 Compare June 26, 2024 22:33
@krishnamtibrewala krishnamtibrewala force-pushed the aie-avoid-bank-conflicts branch from 5fd0102 to 13d6dd9 Compare June 27, 2024 16:34
@krishnamtibrewala krishnamtibrewala mentioned this pull request Jul 1, 2024
Merged
gbossu
gbossu reviewed Jul 2, 2024
View reviewed changes
llvm/lib/Target/AIE/AIE2AddrSpace.h
//
//===----------------------------------------------------------------------===//
//
// This file declares the AIEngine V2 Address Space and DM banks
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Please check with @abnikant or @konstantinschwarz , but I think we are starting to move version-specific files into separate sub-folders. Maybe we should move that new file into a aie2/ folder already.

gbossu
gbossu reviewed Aug 9, 2024
View reviewed changes
Copy link
Collaborator

@gbossu gbossu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of great improvements, thanks for also splitting the code in nice commits! Please look through my remaining commits, we are almost there 💪

llvm/lib/Target/AIE/AIE2Subtarget.cpp Outdated Show resolved Hide resolved
@krishnamtibrewala krishnamtibrewala force-pushed the aie-avoid-bank-conflicts branch 2 times, most recently from 02ee584 to c341c2c Compare August 9, 2024 19:54
gbossu
gbossu reviewed Aug 12, 2024
View reviewed changes
llvm/lib/Target/AIE/AIE2AddrSpace.h
PM, // Address space for Program Memory (PM)
DM, // Address space for Data Memory(DM) includes Bank A, B, C, D
DM_test,
stack, // Address space for stack
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are DM_test and stack actually used somewhere?

Copy link
Collaborator Author

@krishnamtibrewala krishnamtibrewala Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stack could be used as per AIE-API, Not sure about DM_test but we need to have the same enum number for flows to work in future. We can discuss this in more detail.

gbossu
gbossu reviewed Aug 12, 2024
View reviewed changes
llvm/lib/Target/AIE/AIEHazardRecognizer.cpp
}

void FuncUnitWrapper::blockResources() {
Required = ~0;
Reserved = ~0;
Slots = ~0;
// Since the HW stalls in the event of memory bank conflicts, we don't need to
// block the resource. It is overly conservative if we block all memory banks.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a unit test in HazardRecognizerTest to show that blockResources does not touch MemoryBanks?

gbossu
gbossu reviewed Aug 12, 2024
View reviewed changes
Copy link
Collaborator

@gbossu gbossu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at the comments I left, mostly to better disambiguate between TM and DM memories. We are almost there 💪

@krishnamtibrewala krishnamtibrewala force-pushed the aie-avoid-bank-conflicts branch from c341c2c to ceed3ee Compare August 13, 2024 21:47
gbossu
gbossu previously approved these changes Aug 14, 2024
View reviewed changes
Copy link
Collaborator

@gbossu gbossu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great work :)

@krishnamtibrewala krishnamtibrewala force-pushed the aie-avoid-bank-conflicts branch from ceed3ee to f4c07c8 Compare August 14, 2024 14:48
@krishnamtibrewala krishnamtibrewala enabled auto-merge (rebase) August 14, 2024 14:50
@krishnamtibrewala krishnamtibrewala merged commit 88f3fe9 into aie-public Aug 14, 2024
8 checks passed
@gbossu gbossu deleted the aie-avoid-bank-conflicts branch August 15, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martien-de-jong martien-de-jong martien-de-jong left review comments

@andcarminati andcarminati andcarminati left review comments

@stephenneuendorffer stephenneuendorffer stephenneuendorffer left review comments

@gbossu gbossu gbossu approved these changes

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

@abnikant abnikant Awaiting requested review from abnikant abnikant is a code owner

@khallouh khallouh Awaiting requested review from khallouh khallouh is a code owner

@konstantinschwarz konstantinschwarz Awaiting requested review from konstantinschwarz konstantinschwarz is a code owner

@SagarMaheshwari99 SagarMaheshwari99 Awaiting requested review from SagarMaheshwari99 SagarMaheshwari99 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@krishnamtibrewala @stephenneuendorffer @andcarminati @gbossu @martien-de-jong