[Dispatch Creation] Rework dispatch formation logic#21854
Conversation
IanWood1
force-pushed
the
rework_dispatch_creation_logic
branch
5 times, most recently
from
705ee20 to
716fadf
Compare
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
IanWood1
force-pushed
the
rework_dispatch_creation_logic
branch
from
716fadf to
4bb53e9
Compare
MaheshRavishankar
left a comment
MaheshRavishankar
left a comment
There was a problem hiding this comment.
Reading through, did the easy part first, but I still have to understand the FusionTracker object. leaving some comments before I do that.
MaheshRavishankar
left a comment
MaheshRavishankar
left a comment
There was a problem hiding this comment.
Ok did a first pass of the review. Looks really good so far. Have a few question though.
| !consumerOperandMap.isProjectedPermutation()) { | ||
| return failure(); | ||
| } | ||
| AffineMap inverseMap = |
There was a problem hiding this comment.
Might be worth adding some comments here of this form
- producerMap is a map from (producerIterationSpace) -> (operand data space)
- consumerMap is a map from (consumerIterationSpace) -> (operand data space)
- inverseMap is a map from (operand data space) -> (producerIterationSpace)
- composedMap is a map from (producerIterationSpace) -> (consumerIterationSpace)
So now I am not sure I follow what the composedMap.compose(producerMap) is doing
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
MaheshRavishankar
left a comment
MaheshRavishankar
left a comment
There was a problem hiding this comment.
I had some more comments that I dont think have been addressed yet. So not reviewing again. please signal when its ready for review again. (No hurry)
I thought that I had addressed all comments (but forgot to mark them as resolved). If I missed some could you point me to them? |
I unresolved them, but specifically #21854 (comment) and #21854 (comment) |
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
|
Nice cleanup! Lets see if it breaks anything :) |
|
Hi @IanWood1 , This change has introduced a regression and is currently blocking IREE pin version bump : nod-ai/amd-shark-ai#2205. Created an issue for the same: #22055 Is it possible to get the fix for this ? If not, can you please revert the change? cc: @IanNod , @MaheshRavishankar |
Rework FormDispatchRegions to ensure that all outer parallel loops in a dispatch are compatible. The previous implementation did this by ensuring that the producer/consumer's maps were identity along the root op's parallel dimension. However, this breaks down for a generalized DAG dispatch (and requires interchanging indexing maps during the analysis). This change uses input/result indexing maps to track how the root ops outer parallel loops map to each fused operation. Importantly, when a new op is added to the dispatch, all uses/definitions already in the dispatch are used to ensure a single mapping from root to the new op. Closes iree-org#20019 --------- Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
…g#21854)" (iree-org#22058) This reverts commit 087d5b9. Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
…g#21854)" (iree-org#22058) This reverts commit 087d5b9. Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
#22065) Re-land dispatch creation changes with an additional check to make sure that the number of parallel/reduction iterators match (only when fusing reductions together) and that they are not permuted. This fixes the issues in #22053 and #22055 that caused the revert. --------- Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
…g#21854)" (iree-org#22065) Re-land dispatch creation changes with an additional check to make sure that the number of parallel/reduction iterators match (only when fusing reductions together) and that they are not permuted. This fixes the issues in iree-org#22053 and iree-org#22055 that caused the revert. --------- Signed-off-by: Ian Wood <ianwood@u.northwestern.edu> Signed-off-by: Philipp <philipp.weidel@intel.com>
…g#21854)" (iree-org#22065) Re-land dispatch creation changes with an additional check to make sure that the number of parallel/reduction iterators match (only when fusing reductions together) and that they are not permuted. This fixes the issues in iree-org#22053 and iree-org#22055 that caused the revert. --------- Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
3 participants
Rework FormDispatchRegions to ensure that all outer parallel loops in a dispatch are compatible. The previous implementation did this by ensuring that the producer/consumer's maps were identity along the root op's parallel dimension. However, this breaks down for a generalized DAG dispatch (and requires interchanging indexing maps during the analysis).
This change uses input/result indexing maps to track how the root ops outer parallel loops map to each fused operation. Importantly, when a new op is added to the dispatch, all uses/definitions already in the dispatch are used to ensure a single mapping from root to the new op.
Closes #20019