This patch adds a canonicalization pattern for tensor.pad which changes the output type to static at each dimension where the input shape is static and the high and low operands are constants. This corrects an issue arising in Torch-MLIR where pad ops would sometimes introduce dynamic shapes unnecessarily.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
In general looks good to me, although a couple NITS as well as this patterns needs a test.
Please add a lit test to test this around here https://github.com/llvm/llvm-project/blob/cb05c2ffc79eefe74c569263e27fcb5fad167ba3/mlir/test/Dialect/Tensor/canonicalize.mlir#L1111
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp | ||
---|---|---|
2842 | Please add a LIT test to ensure this pattern is always working somewhere here https://github.com/llvm/llvm-project/blob/cb05c2ffc79eefe74c569263e27fcb5fad167ba3/mlir/test/Dialect/Tensor/canonicalize.mlir#L1111 | |
2853 | NIT: You missed a full stop after the end of sentence. | |
2856 | NIT: Can we do for (auto operand : padTensorOp.getOperands()) { ... | |
2861 | NIT: Can we do ShapedType::kDynamic instead of -1. | |
2872 | NIT: You missed a full stop after the end of sentence. | |
2882 | NIT: You missed a full stop after the end of sentence. | |
2886 | NIT: Can we do ShapedType::kDynamic instead of -1. for better readability. | |
2891 | NIT: You missed a full stop after the end of sentence. |
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp | ||
---|---|---|
2842 | I'd also add a test where a padOp actually needs to output dynamic, stays dynamic. |
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp | ||
---|---|---|
2849 | For some reason the comment on test dissapeared. Please add two lit test:
Should be added somewhere here: https://github.com/llvm/llvm-project/blob/cb05c2ffc79eefe74c569263e27fcb5fad167ba3/mlir/test/Dialect/Tensor/canonicalize.mlir#L1111 |
For some reason the comment on test dissapeared. Please add two lit test:
Should be added somewhere here: https://github.com/llvm/llvm-project/blob/cb05c2ffc79eefe74c569263e27fcb5fad167ba3/mlir/test/Dialect/Tensor/canonicalize.mlir#L1111