reviwer comments 2

Signed-off-by: Nirvedh <[email protected]>

Author: nirvedhmeshram

mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp

@@ -2178,22 +2178,22 @@ vectorizePadOpPrecondition(tensor::PadOp padOp,
                                               inputVectorSizes)))
     return failure();
 
-  // Low padding is currently unsupported for the general case as this
-  // would require shifting the results to the right for the low padded dims by
-  // the required amount of low padding. However, we do support low padding if
-  // the dims being low padded have result sizes of 1. The reason is when we
-  // have a low pad on a unit result dim, the input size of that dimension will
-  // be dynamically zero (as the sum of the low pad and input dim size has to be
-  // one) and hence we will create a zero mask as the lowering logic just makes
-  // the mask one for the input dim size - which is zero here. Hence we will
-  // load the pad value which is what we want in this case. If the low pad is
-  // dynamically zero then the lowering is correct as well as no shifts are
-  // necessary.
+  // Padding with non-zero low pad values is not supported, unless the
+  // corresponding result dim is 1 as this would require shifting the results to
+  // the right for the low padded dims by the required amount of low padding.
+  // However, we do support low padding if the dims being low padded have result
+  // sizes of 1. The reason is when we have a low pad on a unit result dim, the
+  // input size of that dimension will be dynamically zero (as the sum of the
+  // low pad and input dim size has to be one) and hence we will create a zero
+  // mask as the lowering logic just makes the mask one for the input dim size -
+  // which is zero here. Hence we will load the pad value which is what we want
+  // in this case. If the low pad is dynamically zero then the lowering is
+  // correct as well as no shifts are necessary.
   if (llvm::any_of(llvm::enumerate(padOp.getLow()), [&](const auto &en) {
         Value padValue = en.value();
         unsigned pos = en.index();
-        std::optional<int64_t> res = getConstantIntValue(padValue);
-        return (!res.has_value() || res.value() != 0) &&
+        std::optional<int64_t> pad = getConstantIntValue(padValue);
+        return (!pad.has_value() || pad.value() != 0) &&
                resultTensorShape[pos] != 1;
       })) {
     LDBG("low pad must all be zero for all non unit dims: " << padOp << "\n");

mlir/test/Dialect/Linalg/vectorization-unsupported.mlir

@@ -305,10 +305,10 @@ module attributes {transform.with_named_sequence} {
 
 // -----
 
-// Low padding is unsupported with the exception of the special case if the
-// result size of the padded dimension is unit. Here `%l0` being a non-zero
-// padding applied to a non-unit result dimension makes this case unsupported.
-func.func @test_masked_vectorize_lowpad(
+// Padding with non-zero low pad values is not supported, unless the corresponding
+// result dim is 1. Here `%l0` being a non-zero low pad applied to a
+// non-unit result dimension makes this case unsupported.
+func.func @tensor_pad_non_zero_low_pad(
   %0 : tensor<?x?xf32>, %h0 : index, %h1 : index, %l0 : index)
     -> tensor<2x4xf32> {
   // expected-error @+3 {{Attempted to vectorize, but failed}}

mlir/test/Dialect/Linalg/vectorization.mlir

@@ -668,8 +668,8 @@ module attributes {transform.with_named_sequence} {
 // This case is supported because low padding `%l0` is applied on
 // a unit dimension which is supported, non unit result dimension low
 // padding is currently unsupported.
-//  CHECK-LABEL: func @test_masked_vectorize_unit_lowpad
-func.func @test_masked_vectorize_unit_lowpad(
+//  CHECK-LABEL: func @test_masked_vectorize_non_zero_low_pad_unit_res_dim
+func.func @test_masked_vectorize_non_zero_low_pad_unit_res_dim(
   %0 : tensor<?x?xf32>, %h0 : index, %h1 : index, %l0 : index)
     -> tensor<1x4xf32>
 {