[Transform] Create memory banks for memories used inside affine parallel loops #7804
Conversation
|
Hey @cgyurgyik , thanks so much for the timely feedback! I have changed based on your review, including changing |
…banking factor is zero
|
Huhh.. I wonder why it has different behavior on CI and locally. I can pass |
|
Different machine? |
jiahanxie353
force-pushed
the
affine-par-bank
branch
from
f7051f7
to
07f57e9
Compare
|
lol finally figure out the issue and passing the CI ready for another round of review again! |
Awesome. Can you resolve any outdated comments please? |
I believe I have resolved these based on your feedback, so I clicked "Resolve conversion" buttons. But please let me know if there's anything still need to be changed! |
lib/Transforms/MemoryBanking.cpp
Outdated
| MemoryBankingPass(const MemoryBankingPass &other) = default; | ||
| explicit MemoryBankingPass( | ||
| std::optional<unsigned> bankingFactor = std::nullopt, | ||
| const std::function<unsigned(mlir::affine::AffineParallelOp)> |
There was a problem hiding this comment.
Why is this function necessary? I'm not actually seeing where this is used
There was a problem hiding this comment.
It's not used indeed.
I referenced those affine loop transformation passes source code and saw them passing customized functions as arguments so that they can perform those customized transformation.
And I thought it could be useful in the future when the banking function is not as simple as taking the mod of the bankingFactor but some other complicated banking function (like TraceBank algorithm)
But I'm fine with removing it for now and introduce again in the future when it's actually used.
Please let me know what you think.
There was a problem hiding this comment.
Yes I'd say let's remove it for now, and then re-add it when we have a definitive reason. LGTM after this is done.
| @@ -0,0 +1,14 @@ | |||
| // RUN: circt-opt %s -split-input-file -memory-banking="banking-factor=0" -verify-diagnostics | |||
There was a problem hiding this comment.
Nit: keep the naming consistent: memory_banking_invalid.mlir or cahnge the other to memory_bank.mlir
|
Thanks! Merging it as the CI has passed |
This patch partition memories used inside affine parallel loops into even banks and replace the old memrefs with new ones throughout the program.