[Mlir-commits] [mlir] 91c0aa5 - [mlir][tensor][nfc] Clarify comments for `createPadHighOp`

[Andrzej Warzynski]

Author: Andrzej Warzynski
Date: 2025-02-22T14:07:00Z
New Revision: 91c0aa5c1038ea9f4b1565b2f9894d5e8b10e85a

URL: https://github.com/llvm/llvm-project/commit/91c0aa5c1038ea9f4b1565b2f9894d5e8b10e85a
DIFF: https://github.com/llvm/llvm-project/commit/91c0aa5c1038ea9f4b1565b2f9894d5e8b10e85a.diff

LOG: [mlir][tensor][nfc] Clarify comments for `createPadHighOp`

While reviewing this code, I realised that the rationale behind the
assert was not very clear, so I updated the comments to clarify it.

Relaxing the assert (i.e., allowing `resType.getNumDynamicDims() !=
dynOutDims.size()`) would require generating a mapping between
`dynOutDims` and the dynamic dimensions of the output tensor. At the
moment, this additional complexity is unnecessary.

To minimize PR noise, I am submitting this without a review. However,
please ping me if you believe this or similar changes should be reviewed
before merging.

Added: 
    

Modified: 
    mlir/include/mlir/Dialect/Tensor/Utils/Utils.h
    mlir/lib/Dialect/Tensor/Utils/Utils.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/include/mlir/Dialect/Tensor/Utils/Utils.h b/mlir/include/mlir/Dialect/Tensor/Utils/Utils.h
index 83cc665b5a4fb..22ca8a99dd7db 100644
--- a/mlir/include/mlir/Dialect/Tensor/Utils/Utils.h
+++ b/mlir/include/mlir/Dialect/Tensor/Utils/Utils.h
@@ -20,16 +20,17 @@ namespace tensor {
 // width is calculated as: resDim - sourceDim.
 //
 // Handling static sizes is trivial. Dynamic dimensions are trickier (*):
-//  1. dynamic input sizes are extracted from `source`
-//  2. for dynamic output dims, there are two options:
-//    2.1 all output dynamic dim sizes are specified in `dynOutDim`,
-//    2.2 `dynOutDim` is empty and the corresponding padding width is set to 0.
+//  1. Dynamic input sizes are extracted from `source` (e.g. via `tensor.dim`).
+//  2. For dynamic output dims, there are two options:
+//    2.1 All output dynamic dim sizes are specified in `dynOutDims`, or
+//    2.2 `dynOutDims is empty - the padding width for all the output dynamic
+//        dims is set to 0.
 //
 // (*) Note that `resType` is just a shape and it only encodes the actual sizes
 // for _static_ dimensions.
 PadOp createPadHighOp(RankedTensorType resType, Value source, Value pad,
                       bool nofold, Location loc, OpBuilder &builder,
-                      SmallVector<Value> dynOutDim = {});
+                      SmallVector<Value> dynOutDims = {});
 
 // Creates dim ops for each dynamic dimension of the ranked tensor argument and
 // returns these as values.

diff  --git a/mlir/lib/Dialect/Tensor/Utils/Utils.cpp b/mlir/lib/Dialect/Tensor/Utils/Utils.cpp
index 52462aae4bc80..c3d56759a896a 100644
--- a/mlir/lib/Dialect/Tensor/Utils/Utils.cpp
+++ b/mlir/lib/Dialect/Tensor/Utils/Utils.cpp
@@ -27,6 +27,8 @@ PadOp mlir::tensor::createPadHighOp(RankedTensorType resType, Value source,
                                     OpBuilder &b,
                                     SmallVector<Value> dynOutDims) {
 
+  // This assumption simplifies the following logic without limiting what's
+  // required _today_. If needed, we can relax it in the future.
   assert(((resType.getNumDynamicDims() == dynOutDims.size()) ||
           dynOutDims.empty()) &&
          "Either none or all output dynamic dims must be specified!");