-
Notifications
You must be signed in to change notification settings - Fork 603
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
Fix operation ordering issue during inlining of operations into dispatch region #5236
Fix operation ordering issue during inlining of operations into dispatch region #5236
Conversation
@benvanik This change could potentially be done in the canonicalizer, but I got a bit lost in the interfaces and wasnt sure if it is actually worth having so much stress on the canonicalizers so as to pull in operands with operands into the dispatch region as well. So I aborted that plan, and just fixed the ordering issue while pulling the ops into the dispatch region during creation itself. |
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.
Overall looks fine - can always extract it out later on if we want. Am concerned about the consistent inversion of argument ordering, though - if it's a reasonable thing to fix that'd be really nice!
iree/compiler/Dialect/Flow/Transforms/DispatchLinalgOnTensors.cpp
Outdated
Show resolved
Hide resolved
std::swap(nextReadyOps, readyOps); | ||
} | ||
|
||
LLVM_DEBUG({ |
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.
this is a lot of debug spew (here and above) - is this enabled with -debug by default?
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 guess so. Is there a way to hide it even further. I can remove it if it is too much.
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.
Using DEBUG_WITH_TYPE. That makes the debug spew guarded by DEBUG_TYPE.
iree/compiler/Dialect/Flow/Transforms/test/dispatch_linalg_on_tensors_dynamic.mlir
Outdated
Show resolved
Hide resolved
iree/compiler/Dialect/Flow/Transforms/test/dispatch_linalg_on_tensors_dynamic.mlir
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.
Overall looks fine - can always extract it out later on if we want. Am concerned about the consistent inversion of argument ordering, though - if it's a reasonable thing to fix that'd be really nice!
I think this is because of the readability of the IR. I saw a comment in the code:
// Reverse the values. This is not for correctness, but more for readability
// of the IR.
iree/compiler/Dialect/Flow/Transforms/DispatchLinalgOnTensors.cpp
Outdated
Show resolved
Hide resolved
iree/compiler/Dialect/Flow/Transforms/DispatchLinalgOnTensors.cpp
Outdated
Show resolved
Hide resolved
iree/compiler/Dialect/Flow/Transforms/DispatchLinalgOnTensors.cpp
Outdated
Show resolved
Hide resolved
iree/compiler/Dialect/Flow/Transforms/DispatchLinalgOnTensors.cpp
Outdated
Show resolved
Hide resolved
iree/compiler/Dialect/Flow/Transforms/DispatchLinalgOnTensors.cpp
Outdated
Show resolved
Hide resolved
iree/compiler/Dialect/Flow/Transforms/DispatchLinalgOnTensors.cpp
Outdated
Show resolved
Hide resolved
iree/compiler/Dialect/Flow/Transforms/DispatchLinalgOnTensors.cpp
Outdated
Show resolved
Hide resolved
I put this |
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.
Thanks, just few nits!
iree/compiler/Dialect/Flow/Transforms/DispatchLinalgOnTensors.cpp
Outdated
Show resolved
Hide resolved
iree/compiler/Dialect/Flow/Transforms/DispatchLinalgOnTensors.cpp
Outdated
Show resolved
Hide resolved
llvm::dbgs() << "\n"; | ||
} | ||
}); | ||
assert(orderedOps.size() == ops.size()); |
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.
Maybe add an error message for the assertion?
@@ -443,6 +523,18 @@ static void tryToTieOperandsAndResults( | |||
} | |||
} | |||
|
|||
void replaceAllUsesWithinDispatchOp(IREE::Flow::DispatchWorkgroupsOp dispatchOp, |
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 this is outside anonymous namespace since other methods are explicitly static. Please also add static
to this method.
cc18320
to
c701be3
Compare
…tch region. In general the operations cloned into a dispatch region could form a DAG. These operations have to be cloned while keeping the order amongst them consistent to not violate use-def chains. This changes adds a method to clone the operations in the right order. Also cleans up the dispatch region creation code. Fixes iree-org#5151.
c701be3
to
afe0ac0
Compare
In general the operations cloned into a dispatch region could form a
DAG. These operations have to be cloned while keeping the order
amongst them consistent to not violate use-def chains. This changes
adds a method to clone the operations in the right order. Also cleans
up the dispatch region creation code.
Fixes #5151.