[mlir] Check if the stride tensor is empty. #76428
Conversation
Added a check to see if the stride tensor is empty. If so then return false for isContiguousSlice function. Possible fix for llvm#74463
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-vector Author: Balaji V. Iyer. (bviyer) ChangesAdded a check to see if the stride tensor is empty. If so then return false for isContiguousSlice function. Possible fix for #74463 Full diff: https://github.com/llvm/llvm-project/pull/76428.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/Vector/Utils/VectorUtils.cpp b/mlir/lib/Dialect/Vector/Utils/VectorUtils.cpp
index 2ad992af989c96..c1c0f5483a6af5 100644
--- a/mlir/lib/Dialect/Vector/Utils/VectorUtils.cpp
+++ b/mlir/lib/Dialect/Vector/Utils/VectorUtils.cpp
@@ -271,7 +271,7 @@ bool vector::isContiguousSlice(MemRefType memrefType, VectorType vectorType) {
return false;
// Cond 1: A contiguous memref will always have a unit trailing stride.
- if (strides.back() != 1)
+ if (strides.empty() || strides.back() != 1)
return false;
// Cond 2: Strides of a contiguous memref have to match the flattened dims.
|
Can you see whether you can simplify the bug as much as possible (simplify the reproducer code) and add it to the tests? Without it, the code that this PR changed might be changed by someone else again in the future and then we need to fix it again. Adding a test will ensure that it is fixed and remains fixed. |
|
Thanks for the fix! We will need a test before this can be merged. |
|
I added a test case here 125d919 |
|
|
||
| // ----- | ||
|
|
||
| // This test is to make sure there is no crash for empty stride. |
There was a problem hiding this comment.
Why not add a proper test instead? That would defend against any future breakage and also test one more quite relevant case.
There was a problem hiding this comment.
I am sorry but I don't understand. What do you mean by a proper test? This test is a simplification of the original test that was breaking. Without my change it will break the same way, and and with my change it will not crash and run to completion. The comment was added to show why the test was added. I also added some CHECK tags with the tests, to make sure it is executing.
There was a problem hiding this comment.
The “check” lines added, unlike for other tests in this file, do not reveal/check/verify/document the expected behaviour for this case. Well, apart from making sure that the transformation doesn’t crash.
Tl,Dr Please add more check lines.
There was a problem hiding this comment.
Thanks for explaining it for me! I added more checks. I also added a check-not for for the error message string that the crash would cause.
the error message that comes during the crash.
|
Thanks for the fix, LGTM! |
As per the docs [1]: ``` In absence of an explicit layout, a memref is considered to have a multi-dimensional identity affine map layout. ``` This patch makes sure that MemRefs with no strides (i.e. no explicit layout) are treated as contiguous when checking whether a particular vector is a contiguous slice of the given MemRef. [1] https://mlir.llvm.org/docs/Dialects/Builtin/#layout Follow-up for llvm#76428.
#76848) As per the docs [1]: ``` In absence of an explicit layout, a memref is considered to have a multi-dimensional identity affine map layout. ``` This patch makes sure that MemRefs with no strides (i.e. no explicit layout) are treated as contiguous when checking whether a particular vector is a contiguous slice of the given MemRef. [1] https://mlir.llvm.org/docs/Dialects/Builtin/#layout Follow-up for #76428.
llvm#76848) As per the docs [1]: ``` In absence of an explicit layout, a memref is considered to have a multi-dimensional identity affine map layout. ``` This patch makes sure that MemRefs with no strides (i.e. no explicit layout) are treated as contiguous when checking whether a particular vector is a contiguous slice of the given MemRef. [1] https://mlir.llvm.org/docs/Dialects/Builtin/#layout Follow-up for llvm#76428.
Added a check to see if the stride tensor is empty. If so then return false for isContiguousSlice function.
Possible fix for #74463