[mlir][Transforms] Add missing check in tosa::transpose::verify()

[DarshanRamakant]

The tosa::transpose::verify() should make sure
that the permutation numbers are within the size of
the input array. Otherwise it will cause a cryptic array
out of bound assertion later.Fix #99513.

[llvmbot]

@llvm/pr-subscribers-mlir-affine
@llvm/pr-subscribers-mlir-tosa
@llvm/pr-subscribers-mlir-linalg

@llvm/pr-subscribers-mlir

Author: None (DarshanRamakant)

Changes

The applyPermutation() utility should make sure
that the permutation numbers are within the size
of the input array. Otherwise it will cause a
cryptic array out of bound assertion later.

---

Full diff: https://github.com/llvm/llvm-project/pull/102099.diff

2 Files Affected:

• (modified) mlir/include/mlir/Dialect/Utils/IndexingUtils.h (+2)
• (added) mlir/test/Conversion/TosaToLinalg/tosa-to-linalg-invalidperm.mlir (+10)

diff --git a/mlir/include/mlir/Dialect/Utils/IndexingUtils.h b/mlir/include/mlir/Dialect/Utils/IndexingUtils.h
index 7849782e5442b..9f3f334d13c89 100644
--- a/mlir/include/mlir/Dialect/Utils/IndexingUtils.h
+++ b/mlir/include/mlir/Dialect/Utils/IndexingUtils.h
@@ -202,6 +202,8 @@ SmallVector<T> applyPermutation(ArrayRef<T> input,
                                 ArrayRef<int64_t> permutation) {
   assert(input.size() == permutation.size() &&
          "expected input rank to equal permutation rank");
+  assert(llvm::all_of(permutation, [&](size_t s) {return s < input.size();}) &&
+          "permutation must be within input bounds");
   auto permutationRange = llvm::map_range(
       llvm::seq<unsigned>(0, input.size()),
       [&](int64_t idx) -> T { return input[permutation[idx]]; });
diff --git a/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg-invalidperm.mlir b/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg-invalidperm.mlir
new file mode 100644
index 0000000000000..85378e55cad77
--- /dev/null
+++ b/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg-invalidperm.mlir
@@ -0,0 +1,10 @@
+// RUN: not --crash mlir-opt %s --pass-pipeline="builtin.module(func.func(tosa-to-linalg-named))" 2>&1 | FileCheck -check-prefix=CHECK %s
+
+func.func @func1() {
+	%arg0 = tensor.empty() : tensor<3x4x5xi32>
+	%1110 = arith.constant dense<[3, 0, 1]> : tensor<3xi32>
+	%143 = tosa.transpose %arg0, %1110: (tensor<3x4x5xi32>, tensor<3xi32>) -> tensor<3x4x5xi32>
+	return 
+}
+
+// CHECK: permutation must be within input bounds

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[cxy-1993]

Thank you for your contribution. Personally, I don't think this assert is necessary. Perhaps we need to check the verify of other dialects.

[DarshanRamakant]

@cxy-1993 @MaheshRavishankar can you please merge this change if there are no other review comments ?

[DarshanRamakant]

Mistakenly closed the pull request, reopening it again.

[DarshanRamakant]

@cxy-1993 @MaheshRavishankar @Mogball can you please merge this PR ?

[cxy-1993]

I maintain my original stance: when permutation is constant, verify doesn't require static shape. So I cant merge this for you. As for the test case you highlighted, I believe it would be more appropriate to modify it, such as by introducing perm as a parameter. And I don't think that's a good test, neither in terms of its style nor its test point design.

[DarshanRamakant]

I maintain my original stance: when permutation is constant, verify doesn't require static shape. So I cant merge this for you. As for the test case you highlighted, I believe it would be more appropriate to modify it, such as by introducing perm as a parameter. And I don't think that's a good test, neither in terms of its style nor its test point design.

Aa suggested, I have removed the dynamic shape check, and modified the failing tests.

[cxy-1993]

Thanks for iterating on this patch, LGTM.

[DarshanRamakant]

Thanks for iterating on this patch, LGTM.

Thanks @cxy-1993, can you please merge the PR ?

[cxy-1993]

Thanks for iterating on this patch, LGTM.

Thanks @cxy-1993, can you please merge the PR ?

Yes. Could you please revise the commit message and PR description before merging? The information in them no longer aligns with the current code.

[DarshanRamakant]

Thanks for iterating on this patch, LGTM.

Thanks @cxy-1993, can you please merge the PR ?

Yes. Could you please revise the commit message and PR description before merging? The information in them no longer aligns with the current code.

Done

[cxy-1993]

I've found another style issue... There's a problem with the CI in the LLVM, this patch isn't the cause of the current CI failure. Please fix this style issue, and then I'll help you merge the code.

[cxy-1993]

Do not use tab

[DarshanRamakant]

Strangely it was no visible in my local vs code. Fixed the issue now.
Yes, the latest CI failures not related, it is happening on all the patches.