-
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
Pdll #976
base: main
Are you sure you want to change the base?
Pdll #976
Conversation
tools/tpp-opt/tpp-opt.cpp
Outdated
|
||
int main(int argc, char **argv) { | ||
mlir::registerAllPasses(); | ||
mlir::tpp::registerTppCompilerPasses(); | ||
mlir::tpp::registerConvertVectorToXsmmPass(); |
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'd expect the pass registration to be already included in registerTppCompilerPasses
.
Is a separate one really needed?
// to the callee to specify the expected rank in the VNNI layout as the rank | ||
// depends on the operations we are dealing with. | ||
bool isInVnniLayout(VnniOperandRank expectedRank, VectorType vector) { | ||
return isInVnniLayout((int64_t)expectedRank, vector); |
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.
nit: use static_cast
@@ -122,17 +213,17 @@ struct IntelAMXTileConfigInsertionPass | |||
: public impl::IntelAMXTileConfigInsertionPassBase< | |||
IntelAMXTileConfigInsertionPass> { | |||
void populateCombinePatterns(RewritePatternSet &patterns) { | |||
patterns.add<IntelAMXTileConfig<xsmm::BrgemmOp, xsmm::BrgemmDispatchOp>>( |
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.
Are we ready to already retire the old lowering?
return xsmm::DataTypeAttr::get(rewriter.getContext(), xsmm::DataType::BF16); | ||
return xsmm::DataTypeAttr::get(rewriter.getContext(), xsmm::DataType::F32); | ||
// Callable object to verify if `operand` has static shape. | ||
struct HasStaticShape { |
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.
Why not just include StructuredOpMatcher.h
for these?
} | ||
|
||
static std::pair<Operation *, Operation *> | ||
buildOpImpl(PatternRewriter &rewriter, Operation *contractOp, Operation *input0, |
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.
Since you have access to rewriter, can you erase other ops here as well?
I wonder if we could have a single buildOp
that on its own tries to fuse consumers to avoid combinatorial explosion of patterns.
rewrite root with{ | ||
let replacement = BuildOp(root, input0, input1, input2); | ||
replace root with (replacement.dispatch, replacement.invoke); | ||
let user = GetUser(replacement.dispatch); |
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.
Not a pdll expert but AFAIK there's no validation or matching on the user here.
I don't think we can just randomly erase it when for example:
%0 = vector.contract
%1 = arith.subf %0, ...
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 you already match for a transfer_write as a consumer in some other patterns so, it's probably just missing here.
FailureOr<vector::ContractionOp> | ||
makeMinorDimensionsInnerMost(RewriterBase &rewriter, | ||
vector::ContractionOp contractOp, unsigned m, | ||
unsigned n, unsigned k, xsmm::DataTypeAttr type); |
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.
xsmm::DataTypeAttr
needs to be removed as it still couples to Xsmm dialect
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.
Actually, I see it still relies on XsmmEnum
in general.
I guess for now we can keep that as it's easy to generate them and we can refactor later.
db785cf
to
51f1df5
Compare
I'm getting pretty different results between linalg vs vector to xsmm: Linalg test case:
Vector test case:
|
After a bit of IR diffing, the current difference comes from dispatch call:
The new call builder add two extra arguments 8th and 9th |
Fixed the issue in the last commit @adam-smnk |
fafa71d
to
413bb90
Compare
namespace xegpu { | ||
class XeGPUDialect; | ||
} // namespace xegpu | ||
|
||
} // namespace mlir | ||
|
||
#include "TPP/Conversion/ConvertVectorToXsmm/ConvertVectorToXsmm.h" |
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 seems out of place. Why is this needed and the others are not?
853e9e4
to
0a86e14
Compare
0a86e14
to
e2bde23
Compare
No description provided.