[MemoryBanking] Support memory banking forGetGlobalOp
#8047
Conversation
lib/Transforms/MemoryBanking.cpp
Outdated
| @@ -88,6 +89,51 @@ MemRefType computeBankedMemRefType(MemRefType originalType, | |||
| return newMemRefType; | |||
| } | |||
|
|
|||
| // Updates n-dimensional index array `ndIndex` in-place by decoding the flat | |||
| // index `linIndex` given the shape of the array. Assume row-major order. | |||
| void decodeIndex(int64_t linIndex, ArrayRef<int64_t> shape, | |||
There was a problem hiding this comment.
Nit: just return a SmallVector<int64_t> instead of passing it as a reference?
lib/Transforms/MemoryBanking.cpp
Outdated
| auto globalOp = | ||
| dyn_cast_or_null<memref::GlobalOp>(SymbolTable::lookupSymbolIn( |
There was a problem hiding this comment.
If you're doing dyn_cast_or_null, we should have an assert here to make sure its not null. If you're absolutely sure it exists, then we should just use cast
lib/Transforms/MemoryBanking.cpp
Outdated
| for (size_t bankCnt = 0; bankCnt < bankingFactor; ++bankCnt) { | ||
| if (bankCnt == 0) { | ||
| // initialize globalOp and getGlobalOp's insertion points | ||
| builder.setInsertionPointAfter(globalOp); | ||
| globalOpsInsertPt = builder.saveInsertionPoint(); | ||
| builder.setInsertionPointAfter(getGlobalOp); | ||
| getGlobalOpsInsertPt = builder.saveInsertionPoint(); | ||
| } |
There was a problem hiding this comment.
Nit: can we move this outside the loop, and if you want to avoid this when bankingFactor == 0, then just add an if statement as an early exit?
| @@ -1,4 +1,5 @@ | |||
| // RUN: circt-opt %s -split-input-file -memory-banking="banking-factor=2 dimension=1" | FileCheck %s --check-prefix RANK2-BANKDIM1 | |||
| // RUN: circt-opt %s -split-input-file -memory-banking="banking-factor=2" | FileCheck %s --check-prefix GETGLOBAL | |||
There was a problem hiding this comment.
You may have handled this in an earlier PR, but what about corner cases, e.g., banking-factor=0, dim_size % banking-factor != 0, etc.?
There was a problem hiding this comment.
Yes, in these cases, it will early signal failure:
circt/lib/Transforms/MemoryBanking.cpp
Line 381 in 4a062a2
or raise an assertion error:
circt/lib/Transforms/MemoryBanking.cpp
Line 80 in 4a062a2
| @@ -125,6 +171,76 @@ SmallVector<Value, 4> createBanks(Value originalMem, uint64_t bankingFactor, | |||
| banks.push_back(bankAllocaOp); | |||
| } | |||
| }) | |||
| .Case<memref::GetGlobalOp>([&](memref::GetGlobalOp getGlobalOp) { | |||
There was a problem hiding this comment.
How difficult would it be to place all this code in a separate function and just call it here? It is quite large, but unsure if that would require too many arguments.
…ference and update it in-place; and some style changes in the function
…yncast; and some style changes
jiahanxie353
force-pushed
the
getglobal-membank
branch
from
d1e08ae to
1561235
Compare
|
Thanks @cgyurgyik for the review and suggestions! I've finished modifying the code based on the request. And it's ready for another round of review! |
This patch supports memory banking for multi-dimensional
GetGlobalOps and restructure the data in their correspondingGlobalOps by the banking scheme (such as the banking factor and the dimension).