This verification is to check if the indices for static shaped operands on linalgOps access out of bound memory or not. For dynamic shaped operands, we would be able to check it on runtime stage.
Found 3 memory violation testcases, and modified them
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
435 | Check the return value | |
436 | The LLVM style is for (unsigned i = 0, e = ...; i < e; ...) | |
439–440 | This can be getShapedOperandTypes()[i]. | |
443 | You should check if there are any symbols before calling compose method. Otherwise, it will hit an assertion in the method. | |
445 | Why check with >=? Can we check if they are the same, ie with ==? You can know the exact shape if you have loop boundary. | |
mlir/test/Dialect/Linalg/reshape_linearization_fusion.mlir | ||
171–172 | Below is more straightforward to me: #map2 = affine_map<(d0, d1, d2) -> (d2, d0, d1)> #map3 = affine_map<(d0, d1, d2) -> (d0, d1, d2)> Does it work? | |
173 | please help fix this typo since you touch this section. s/permultation/permutation |
Probably need a test that fails as well and verify the error message. You can use -verify-diagnostics on the command line, and match the error using // expected-error . See other tests that have this.
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
436 | Alternatively, for (int i : llvm::seq<int>(0, (*loopRanges).size()) But even better any time you don't need the index: for (int64_t &range : *loopRanges) range -= 1; Also good to know the alternative when you need the index but iterate over a range: for (auto &indexedRange : llvm::enumerate(*loopRanges)) { // indexedRange.index() for the index // indexedRange.value() for the value |
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
436 | Wow It's very helpful! Thanks for sharing the tips | |
443 | I think it was already checked in line 327 when constructing indexingMaps,, isn't it? | |
445 | Could you give me more hint? I thought we should check the upper bound of loops and the size of shaped operands since we have to detect access out of boundary. Or do you mean the size and loop range should be same? | |
mlir/test/Dialect/Linalg/reshape_linearization_fusion.mlir | ||
171–172 | With your suggestion, the indices are less than the sizes, but it is not match. I mean 3x35 shape matches 3x18 loop range, and 5x7x3 shape matches 4x2x2 loop range.. |
- Updating D98390: Added static verification for Linalg Ops. #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Reflected small code styles
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
445 | Yes, they should be the same. It does not make sense to me if one is larger. Otherwise the behavior is undefined. Say that both affine_map is (d0) -> (d0). One shape is [10], and another is [20]. It can raise the issue or not raise issue based on how you infer the shape. If you get the upper bound from the first operand, then the check passes. If you get the upper bound from the second operand, then the check fails. Does it make sense? | |
mlir/test/Dialect/Linalg/reshape_linearization_fusion.mlir | ||
171–172 | I do not understand what the issue is here. d0, d1, d2 will map to three numbers. This is just like what symbol maps to what value. I don't see the difference if you swap d1 and d2. I actually don't understand what you are saying like 3x18 loop range and 4x2x2 loop range. Could you explain more? What is the IR if you go with the suggestion? |
mlir/test/Dialect/Linalg/reshape_linearization_fusion.mlir | ||
---|---|---|
171–172 | Okay, I removed 'range -=1 statement' according to your comment above (not committed yet), and printed out each rank's size of the shapes and loop ranges. Here is IR with the suggestion.inhoseo@inho:~/llvm-project/build/bin$ ./mlir-opt linalg_test.mlir -linalg-fold-reshape-ops-by-linearization -verify-diagnostics func @generic_op_120_permultation_reshape_producer_fusion(%arg0: tensor<3x35xf32>) -> tensor<5x7x3xf32> { %0 = linalg.init_tensor [5, 7, 3] : tensor<5x7x3xf32> %1 = linalg.generic {indexing_maps = [#map0, #map1], iterator_types = ["parallel", "parallel", "parallel"]} ins(%arg0 : tensor<3x35xf32>) outs(%0 : tensor<5x7x3xf32>) { ^bb0(%arg1: f32, %arg2: f32): // no predecessors linalg.yield %arg1 : f32 } -> tensor<5x7x3xf32> return %1 : tensor<5x7x3xf32> } }-convert-linalg-to-loops does not work with other options here.. |
@inho9606, please add invalid ops to test/Dialect/Linalg/invalid.mlir
mlir/test/Dialect/Linalg/reshape_linearization_fusion.mlir | ||
---|---|---|
171–172 |
I don't get the reason to remove 'range -=` statement'. But this seems unrelated, I will take a look once you upload the patch.
I think there are two issues.
#map2 = affine_map<(d0, d1, d2) -> (d1, d2, d0)> #map3 = affine_map<(d0, d1, d2) -> (d0, d1, d2)> %2 = linalg.generic {indexing_maps = [#map2, #map3], iterator_types = ["parallel", "parallel", "parallel"]} ins(%0 : tensor<3x5x7xf32>) outs(%1 : tensor<5x7x3xf32>) { ^bb0(%arg2: f32, %arg3: f32): // no predecessors linalg.yield %arg2 : f32 } -> tensor<5x7x3xf32> The indexing maps for the generic op should be #map2 = affine_map<(d0, d1, d2) -> (d2, d0, d1)> #map3 = affine_map<(d0, d1, d2) -> (d0, d1, d2)> The indexing maps that you provide is also correct. This is just the order of loops problem.
The indexing maps in the output are #map0 = affine_map<(d0, d1, d2) -> (d1, d0 + d2 * 7)> #map1 = affine_map<(d0, d1, d2) -> (d0, d1, d2)> %1 = linalg.generic {indexing_maps = [#map0, #map1], iterator_types = ["parallel", "parallel", "parallel"]} ins(%arg0 : tensor<3x35xf32>) outs(%0 : tensor<5x7x3xf32>) { ^bb0(%arg1: f32, %arg2: f32): // no predecessors linalg.yield %arg1 : f32 } -> tensor<5x7x3xf32> while I think they should be #map0 = affine_map<(d0, d1, d2) -> (d2, d1 + d0 * 7)> #map1 = affine_map<(d0, d1, d2) -> (d0, d1, d2)> With your fix, you are hiding the issue in (2). I think we can go with your fix. Then Mahesh or I will fix the bug and add one more test for it. @mravishankar fyi, the input of the test case was wrong I think. |
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
445 | Yes, if d0 matched (10) already and d0 tries to match (20), then it should fail. | |
mlir/test/Dialect/Linalg/reshape_linearization_fusion.mlir | ||
173 | Could you give me more detail what you meant by 's/permultation/permutation'? |
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
445 | Yes, if d0 matched (10) already and d0 tries to match (20), then it should fail. |
mlir/test/Dialect/Linalg/reshape_linearization_fusion.mlir | ||
---|---|---|
173 | Oops, I meant to replace permultation with permutation. |
- Updating D98390: Added static verification for Linalg Ops. #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Added invalid op in invalid.mlir
- Updating D98390: Added static verification for Linalg Ops. #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Updated verification condition, and fixed some more testcases
- Updating D98390: Added static verification for Linalg Ops. #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Removed comments and changed variable names
Inho and I had an offline discussion today, and we found that this doesn't work in some cases. E.g., if there is an affine_map is affine_map<(i) -> (10 - i)>. I will wait for Inho's fix and review it later.
- Updating D98390: Added static verification for Linalg Ops. #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Added ignoring case that has zero as it's laast inferred accessing index.
- Updating D98390: Added static verification for Linalg Ops. #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Applied clang-format to LinalgInterfaces.cpp
- Updating D98390: Added static verification for Linalg Ops. #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Applied rebase to top
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
435 | nit: rename it to hasDynamicRange? | |
439–445 | The logic is mixed in this loop. You can use llvm::any_of and it's better to have a comment on why you subtract loop ranges. E.g., bool hasDynamicRange = llvm::any_of(...) for (auto ...) range -= 1; The second line can even put to the body of if-statement. And in this case, we can write something like: if (llvm::any_of(...)) { for (auto ...) range -= 1; ... } | |
446 | Add a comment on why? We actually can verify the below snippet right? linalg.matmul ins(%A, %B: memref<2x?xf32>, memref<?x4xf32>) outs(%C: memref<2x4xf32>) Is the reason that this is too expensive to check? | |
447–451 | Let's follow LLVM style, use llvm::enumerate(inalgOp.getShapedOperandTypes()). | |
452 | style nit: for (auto j : llvm:seq<unsigned>(0, shapedOperand.getRank())) | |
453–456 | I feel we should have a more detailed description because this is not well-defined yet. How about: Ignore the case that the inferred last index is zero. The indexing is either increasing or decreasing in Linalg, ie, the last index will be `0` or `size-1`. We only check the cases that are non-zero because most of cases are increasing and it is too expensive to find the shape of decreasing cases. | |
459–465 | How about: inferred XXX shaped operand shape's dimension XXX to be XXX but found XXX This looks shorter and cleaner to me. |
- Updating D98390: Added static verification for Linalg Ops. #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Updated some comments and code styles
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
439–445 | Thanks for the useful feature. I think none-of would be more clear to read, so used it instead of any_of. But please let me know if I made something wrong please since it was my first time to use this. | |
447–451 | I think we still need index i to compare loop ranges and shaped sizes, and to indicate which operand has an error in debug messages. So I just made this line like what you suggested for index j below instead. |
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
447–451 | llvm::enumerate will return the index and the object. for (auto &en : llvm::enumerate(linalgOp.getShapedOperandTypes())) { // en.index() for the index // en.value() for the value In this case en.value() will be i. | |
455 | ||
461–462 | It is weird to me that start with "Detected Invalid shaped operand.". I don't see other verify methods have this kind of things. Can we delete it? And they are supposed to be sentence fragments. Errors reported via emitError or emitOpError should start with lower case. |
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
447–451 | typo... I meant en.index will be i. |
- Updating D98390: Added static verification for Linalg Ops. #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Applied LLVM style and fixed comments
- Updating D98390: Added static verification for Linalg Ops. #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Check complicated cases; pass if indices are in-of-bound of shapes.
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
446 | I don't think the comment explains my example. You will skip the check when some dims are dynamic and some dims are static. My example can be checked. Is there a reason for not checking it? | |
446 | We don't intend to modify any values right? Let's use const auto &en. | |
450 | Let's use auto j. LLVM style says that use auto with initializers like cast<Foo>(...) or other places where the type is already obvious from the context. llvm::seq already spells the type. https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | |
452 | type: Replace inffered with inferred. | |
455 | I actually don't know the English of this symbol. It is probably a grave accent. You can type it with the key above Tab key. It is also on the left hand side of 1. | |
461–466 | The walk is not needed here. I think it is a tree-based structure. If the kind of an Affine expression is DimId, it won't have any children. If it is not, it is "complicated" in this case. There are couple ways to do it:
if (dyn_cast<AffineDimExpr>(expr)) { // not complicated case } else { // complicated case }
The first way is more common I think. | |
467–481 | Couple issues.
| |
mlir/test/Dialect/Linalg/invalid.mlir | ||
707–714 ↗ | (On Diff #332587) | Add one more invalid test case for another case. |
- Updating D98390: Added static verification for Linalg Ops. #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Updated code style and comments..
Added an invalid testcase.
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
455 | Oh grabe is correct! Actually the screen reader says '>' as 'grater than', and says '`' as grabe. It is similar, so my ears thought of them as same things.. LOL |
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
465–479 | There are four en.value().getDimSize(j), maybe have a variable for it? | |
473–476 | The error message is not reasonable, please fix it. should it be greater than? | |
mlir/test/Dialect/Linalg/invalid.mlir | ||
719 ↗ | (On Diff #332858) | This error message is weird. xxx be less than or equal to 4 but found 3. Doesn't 3 less than 4? |
mlir/test/Dialect/Linalg/tile-indexed-generic.mlir | ||
86 ↗ | (On Diff #332858) | accidental change? please revert it. |
- Updating D98390: Added static verification for Linalg Ops. #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Fixed wierd comment.. and fixed minor style
mlir/test/Dialect/Linalg/invalid.mlir | ||
---|---|---|
719 ↗ | (On Diff #332858) | Oops it is definitely my fault.. Thanks |
mlir/test/Dialect/Linalg/tile-indexed-generic.mlir | ||
86 ↗ | (On Diff #332858) | Ah,, I often use tab key to explore the screen, but sometimes it is typed as character and I miss it easily.. I need to be more careful. Anyway thanks for letting me know! |
- Updating D98390: Added static verification for Linalg Ops. #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Fixed clang-format issue
It looks like there is a failure in tile-and-fuse-tensors.mlir. Could you check the file/test, so we can make bots happy?
- Updating D98390: Added static verification for Linalg Ops. #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Rebased the patch to the top of tree, and fixed one test in tile-and-fuse-tensors
- Updating D98390: Added static verification for Linalg Ops. #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
I think this version is more reasonable comparing to other cases
Check the return value