-
Notifications
You must be signed in to change notification settings - Fork 608
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
[Codegen][GPU] Make operand promotion controlled by lowering config #18576
Conversation
c388bab
to
49a8ee7
Compare
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.
Looks good, but would be nice to try to organize the lowering config stuff a bit.
attrs.emplace_back(StringAttr::get(context, "promote_operands"), | ||
b.getI64ArrayAttr({0, 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.
There are quite a few hard coded strings here now. Maybe you can expose getTilingLevelName
, and use it for the tiling levels, and something similar for "mma_kind"
and "promote_operands"
. Or add some setters for the different attribute fields and use those.
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.
Yeah good idea, I don't like the floating strings either. I think I'll do this in a separate PR because that is mostly unrelated to this change.
compiler/src/iree/compiler/Codegen/LLVMGPU/test/ROCDL/pipeline_tile_and_fuse.mlir
Show resolved
Hide resolved
compiler/src/iree/compiler/Codegen/Dialect/GPU/IR/IREEGPUAttrs.cpp
Outdated
Show resolved
Hide resolved
@@ -1366,6 +1369,17 @@ IREE::GPU::MmaInterfaceAttr LoweringConfigAttr::getMmaKind() const { | |||
return getAttributes().getAs<IREE::GPU::MmaInterfaceAttr>(kMmaKindName); | |||
} | |||
|
|||
constexpr StringLiteral kPromoteOperandsName = "promote_operands"; |
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.
Should we put this in a header file for the iree gpu dialect? I'd appreciate having some brief documentation explaining what these attribute mean without having to parse through the codegen logic.
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 changed it to a setter since both you and Max commented about it. I'm going to send a follow up soon to do the same to the TilingLevel strings.
compiler/src/iree/compiler/Codegen/Dialect/GPU/TargetUtils/ConfigUtils.cpp
Show resolved
Hide resolved
Promoting the operands of a matmul is optional and best to control through the lowering config rather than based on on the fly analysis. This gives greater flexibility for adding support for other operations too (like promotion of another kind of contraction or convolution like op without have to always extend this pass).
49a8ee7
to
c154b8b
Compare
Promoting the operands of a matmul is optional and best to control through the lowering config rather than based on on the fly analysis. This gives greater flexibility for adding support for other operations too (like promotion of another kind of contraction or convolution like op without have to always extend this pass).