-
Notifications
You must be signed in to change notification settings - Fork 29
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.
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
[ObjectFifo][NFC] Refactor DmaUtils + SplitLogicalObjectFifoForReuse #759
Conversation
-- This commit adds refactoring of few utilities in DmaUtils as well as a few involved in SplitLogicalObjectFifosForReuse pass. -- This is required for the follow-up PR that adds a new pass `--iree-amdaie-combine-logical-objectfifos-for-connection-reuse`. Signed-off-by: Abhishek Varma <[email protected]>
SmallVector<OpFoldResult, 4> staticL3AsSourceSizes = | ||
SmallVector<OpFoldResult> staticL3AsSourceSizes = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relaying here the review comment : #755 (comment) (@jtuyls )
Two reasons :-
- The second argument to
SmallVector<arg1, arg2>
used to be mandatory but stopped being so - I came across this relaxed constraint in one of the upstream contribution's review comments. - In case of "splitting" we start creation of a new L2 logicalobjectFifo via L2->L1's route. In case of "combing" we start it from L3->L2. The function
createNewLogicalObjectFifo
declaration complains because :-
a.SmallVector<arg1>
!=SmallVector<arg1, 4>
b.SmallVector<arg1, 4>
!=SmallVector<arg1, 6>
Therefore the only way to circumvent point 2z, that I knew of, was to remove this which aligned well with point 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's still useful to specify the number of inlined elements in the vector as we know it will usually be less than 4 and not much more: SmallVector<OpFoldResult, 4>
. createNewLogicalObjectFifo
then has to accept a SmallVectorImpl
instead of a SmallVector
to avoid the issue in 2).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I've addressed it in the latest push. Please take a look.
Hi @jtuyls , based on #755 (review) I'm segregating #755 into two PRs. This concerns NFC changes required to have the actual pass (#760) implemented. |
compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/AMDAIELogicalObjFifoSplittingUtils.cpp
Show resolved
Hide resolved
compiler/plugins/target/AMD-AIE/iree-amd-aie/Transforms/AMDAIELogicalObjFifoSplittingUtils.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one nit below
-- This commit adds refactoring of few utilities in DmaUtils as well as a few involved in SplitLogicalObjectFifosForReuse pass.
-- This is required for the follow-up PR that adds a new pass
--iree-amdaie-combine-logical-objectfifos-for-connection-reuse
.Signed-off-by: Abhishek Varma [email protected]