Page MenuHomePhabricator

Added static verification for Linalg Ops.
ClosedPublic

Authored by inho9606 on Mar 10 2021, 9:00 PM.

Details

Summary

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  1. Updating D98390: Added static verification for Linalg Ops. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

Added invalid op in invalid.mlir

  1. Updating D98390: Added static verification for Linalg Ops. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

Updated verification condition, and fixed some more testcases

  1. Updating D98390: Added static verification for Linalg Ops. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

Removed comments and changed variable names

hanchung requested changes to this revision.Mar 16 2021, 5:39 AM

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.

This revision now requires changes to proceed.Mar 16 2021, 5:39 AM
  1. Updating D98390: Added static verification for Linalg Ops. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

Added ignoring case that has zero as it's laast inferred accessing index.

  1. Updating D98390: Added static verification for Linalg Ops. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

Applied clang-format to LinalgInterfaces.cpp

  1. Updating D98390: Added static verification for Linalg Ops. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

Applied rebase to top

hanchung requested changes to this revision.Mar 18 2021, 9:58 AM
hanchung added inline comments.
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp
437

nit: rename it to hasDynamicRange?

441–447

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;
  ...
}
448

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?

449–453

Let's follow LLVM style, use llvm::enumerate(inalgOp.getShapedOperandTypes()).

454

style nit:

for (auto j : llvm:seq<unsigned>(0, shapedOperand.getRank()))
455–458

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.
461–467

How about:

inferred XXX shaped operand shape's dimension XXX to be XXX but found XXX

This looks shorter and cleaner to me.

This revision now requires changes to proceed.Mar 18 2021, 9:58 AM
  1. Updating D98390: Added static verification for Linalg Ops. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

Updated some comments and code styles

inho9606 added inline comments.Mar 18 2021, 9:14 PM
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp
441–447

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.

449–453

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.

hanchung requested changes to this revision.Mar 18 2021, 11:01 PM
hanchung added inline comments.
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp
449–453

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.

457
463–464

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.

This revision now requires changes to proceed.Mar 18 2021, 11:01 PM
hanchung added inline comments.Mar 18 2021, 11:05 PM
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp
449–453

typo... I meant en.index will be i.

  1. Updating D98390: Added static verification for Linalg Ops. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

Applied LLVM style and fixed comments

inho9606 added inline comments.Mar 19 2021, 2:08 AM
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp
449–453

Oh I didn't know if it has en.index(). thanks!

457

Sorry, I didn't catch what you wanted to mean because my screen reader does not recognize your comment. I think it might be because of starting with //.. Could you add the comment again?

  1. Updating D98390: Added static verification for Linalg Ops. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

Check complicated cases; pass if indices are in-of-bound of shapes.

hanchung requested changes to this revision.Mar 23 2021, 11:12 AM
hanchung added inline comments.
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp
448

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?

448

We don't intend to modify any values right? Let's use const auto &en.

452

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

454

type: Replace inffered with inferred.

457

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.

463–468

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:

  1. you can try to dyn_cast it to AffineDimExpr e.g.,
if (dyn_cast<AffineDimExpr>(expr)) {
  // not complicated case
} else {
  // complicated case
}
  1. Just check if the kind is a DimID by expr.getKind().isa<>(mlir::AffineExprKind::DimId).

The first way is more common I think.

469–483

Couple issues.

  1. 0-base is more common. Let's not go with 1-base.
  2. Add comments. People won't understand why checking it this way.
  3. It's better to have a variable for indices[j] + 1, maybe inferredSize?
mlir/test/Dialect/Linalg/invalid.mlir
707–714

Add one more invalid test case for another case.

This revision now requires changes to proceed.Mar 23 2021, 11:12 AM
  1. Updating D98390: Added static verification for Linalg Ops. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

Updated code style and comments..
Added an invalid testcase.

inho9606 added inline comments.Mar 23 2021, 8:50 PM
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp
457

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

hanchung requested changes to this revision.Mar 25 2021, 12:06 AM
hanchung added inline comments.
mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp
467–481

There are four en.value().getDimSize(j), maybe have a variable for it?

475–478

The error message is not reasonable, please fix it. should it be greater than?

mlir/test/Dialect/Linalg/invalid.mlir
719

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

accidental change? please revert it.

This revision now requires changes to proceed.Mar 25 2021, 12:06 AM
  1. Updating D98390: Added static verification for Linalg Ops. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

Fixed wierd comment.. and fixed minor style

inho9606 added inline comments.Mar 25 2021, 3:53 AM
mlir/test/Dialect/Linalg/invalid.mlir
719

Oops it is definitely my fault.. Thanks

mlir/test/Dialect/Linalg/tile-indexed-generic.mlir
86

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!

  1. Updating D98390: Added static verification for Linalg Ops. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

Fixed clang-format issue

hanchung added a comment.EditedMar 25 2021, 6:05 AM

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?

  1. Updating D98390: Added static verification for Linalg Ops. #
  2. Enter a brief description of the changes included in this update.
  3. 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

  1. Updating D98390: Added static verification for Linalg Ops. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

I think this version is more reasonable comparing to other cases

hanchung accepted this revision.Mar 29 2021, 5:00 AM

Thanks!

This revision was not accepted when it landed; it landed in state Needs Review.Mar 30 2021, 7:11 AM
This revision was automatically updated to reflect the committed changes.