The current static checker for linalg does not work on the decreasing index cases well. So, this is to Update the current static bound checker for linalg to cover decreasing index cases.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks for adding this!
Please add some tests to invalid.mlir
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
470–472 | We don't have to declare startLoopRangeValues above. Instead we can declare it here. E.g., SmallVector<int64_t, 4> startLoopRangeValues((*endLoopRangeValues).size(), 0); | |
491 | I think we don't need to check if shapedDimSize > 0. It won't be dynamic dim because you already check it above. | |
492–500 | Do we really need mapStr here? Can't we use indexingMaps[en.index()] directly? | |
504–517 | How about having a inferredDimSize = std::max(firstIndex, lastIndex) + 1, so we can simplify the check. In this case, we don't have to check two times for the last part. |
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
470–472 | Oh, I remember it caused a linking error when I tried this first, but it works now.. I think I did something wrong. Thanks | |
491 | Yes, but there is a testcase using shapes like memref<0xf32>.. If the size is 0, then the index should be -1 which would be always an error by this condition. #accesses = [ affine_map<(i) -> (i)> ] #trait = { indexing_maps = #accesses, iterator_types = ["parallel"] } func @dce_zero_memref(%arg0 : memref<0xf32>, %arg1: tensor<0xf32>) -> tensor<0xf32> { // memref<0x32> is expected to be dce'ed linalg.copy(%arg0, %arg0): memref<0xf32>, memref<0xf32> // tensor<0xf32> cannot be dce'ed %1 = linalg.generic #trait outs(%arg1 : tensor<0xf32>) { ^bb(%0: f32) : linalg.yield %0 : f32 } -> tensor<0xf32> return %1: tensor<0xf32> } | |
492–500 | I couldn't find the way to use indexingMaps[en.index()] directly in the message.. I think it should be something convertable to string for the message. But the indexingMap is affineMap, so I thought I would need a method to convert it, but couldn't find.. Could you help me? | |
504–517 | You are right. Thanks for reminding me max method.. |
- Updating D101026: Update static bound checker for Linalg to cover decreasing cases #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Reflected the comments and added invalid test cases.
I think you accidentally delete some invalid tests, could you add them back?
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
473–477 | Let's rename them as startIndices and endIndices | |
491 | I see. Seems to be corner case. It will be killed, though. Let's skip the check for this directly, we don't have to make it work with below logic. | |
492–500 | I thought it is streamable and you could pass it to emitError directly. Does this work? linalgOp.emitError("unexpected result less than 0 at expression #") << j << " in affineMap\n" << indexingMaps[en.index()]; | |
mlir/test/Dialect/Linalg/invalid.mlir | ||
781–785 | Can we use a simpler test case? This looks like an matmul with weird accessing. Let's create a reverse op test case? |
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
491 | Okay, then I'll ignore the case that dimsize is 0. | |
492–500 | No it does not work,, it causes no match error.. | |
mlir/test/Dialect/Linalg/invalid.mlir | ||
781–785 | Okay sounds better! I'll update it next Monday. |
- Updating D101026: Update static bound checker for Linalg to cover decreasing cases #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Renamed variables, fixed the logic for dimsize 0, and fixed an invalid testcase.
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
490–491 | I think we can move this above the if-statement because you declare it right after use it. | |
500–502 | I see, thanks for trying this out. I think we can delete "affineMap\n". Having a new line is a bit weird to me, and you did not check the affine map in the test. | |
514 | update the message? We should remove "or equal to"? |
Uh, I think I found a case that this solution can't check, which does not have its maximum and minimum indices with the start and end indices. For example, if the affine_map is <(d0, d1) -> (d1 - d0)> and the ranges of d0 and d1 are 0 and 1, then this expression has
0 when d0 is 0 and d1 is 0,
1 when d0 is 0 and d1 is 1,
-1 when d0 is 1 and d1 is 0,
0 when d0 is 1 and d1 is 1.
I wanted to share this case.. And I think we should substitude
the maximum values for negative dims, and the minimum values for positive dims in order to get the real minimum indices. For the maximum indices, we need the minimum values for negative dims, and the maximum values for positive dims..
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
490–491 | You are right, I missed the point. Thanks | |
500–502 | Ah I see | |
514 | Hmm, I think we should remain it because: |
- Updating D101026: Update static bound checker for Linalg to cover decreasing cases #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Moved declaring a variable and updated the error message
I never see this kind of statement before, and I highly suspect that this will be the case. For now, let's treat it as invalid. We can revert it if this is really the case in the future.
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp | ||
---|---|---|
514 | Yes, you're right. Sorry for the bad review comment. |
Ah, I can't see the conflicts in my repository.. Rebase was successfully done. Could you tell me what files have conflicts?
Thanks
I can not patch this, and the bot can not do it either. If you rebase successfully, you can update the patch. arc will ask you to provide some description and update the patch I think.
- Updating D101026: Update static bound checker for Linalg to cover decreasing cases #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment.
Rebased for merge?
I updated the patch with arc,, could you please check if it works again? Interesting..
I think I can make the checker cover this interesting case as well using affineExpr to get maximum and minimum values. I'd like to try to push it more to make it as perfect as possible. Can I try to fix it more..?
Hey Inho, the bot still could not apply the patch. The diff details show that it's failed. And I can't patch it locally either.
Could you try to run arc patch D1010260 on main branch?
And you will see
This diff is against commit a2c783f910b6034118f3d18dcd1519e9ff74f43f, but the commit is nowhere in the working copy. Try to apply it against the current working copy state? (a67a377014ceaaed55b2ba2259e080bc3fc42b43 \ D101610)
I think I can make the checker cover this interesting case as well using affineExpr to get maximum and minimum values. I'd like to try to push it more to make it as perfect as possible. Can I try to fix it more..?
It's a rare case. If you'd like to push it more, I'd suggest to create another patch for it.
- Updating D101026: Update static bound checker for Linalg to cover decreasing cases #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment
#i
Try to fix the apply issue
We don't have to declare startLoopRangeValues above. Instead we can declare it here. E.g.,