This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Tensor] Canonicalize dynamic tensor.pad ops with constant inputs
ClosedPublic

Authored by gpetters94 on Feb 1 2023, 9:17 PM.

Details

Summary

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.

Diff Detail

Event Timeline

gpetters94 created this revision.Feb 1 2023, 9:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2023, 9:17 PM
gpetters94 requested review of this revision.Feb 1 2023, 9:17 PM

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
2853

NIT: You missed a full stop after the end of sentence.
Extract the static info from the high and low operands.

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.

raikonenfnu added inline comments.Feb 2 2023, 10:13 AM
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
2842

I'd also add a test where a padOp actually needs to output dynamic, stays dynamic.

gpetters94 marked 7 inline comments as done.

Fixed the failing tests.

raikonenfnu added inline comments.Feb 3 2023, 12:37 AM
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
2914

For some reason the comment on test dissapeared. Please add two lit test:

  1. test where a padOp actually needs to output dynamic, stays dynamic
  2. test where a padOp is outputting dynamic result but can actually be static.

Should be added somewhere here: https://github.com/llvm/llvm-project/blob/cb05c2ffc79eefe74c569263e27fcb5fad167ba3/mlir/test/Dialect/Tensor/canonicalize.mlir#L1111

Style NIT: Please update the commit name to have [mlir][Tensor] prefix.

gpetters94 updated this revision to Diff 494718.Feb 3 2023, 1:35 PM
gpetters94 retitled this revision from Canonicalize dynamic tensor.pad ops with constant inputs to [mlir][Tensor] Canonicalize dynamic tensor.pad ops with constant inputs.

Added a test, fixed bug in the code. Should be good to go now.

This revision is now accepted and ready to land.Feb 3 2023, 1:39 PM
This revision was landed with ongoing or failed builds.Feb 3 2023, 1:44 PM
This revision was automatically updated to reflect the committed changes.