This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Avoid tensor canonicalizer crash on negative dimensions
ClosedPublic

Authored by rikhuijzer on May 27 2023, 3:28 AM.

Diff Detail

Event Timeline

rikhuijzer created this revision.May 27 2023, 3:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2023, 3:28 AM
rikhuijzer requested review of this revision.May 27 2023, 3:28 AM
ftynse added a subscriber: ftynse.May 30 2023, 7:12 AM

It's strange that this is even allowed by the op verifier. Can we (also) change the verifier of tensor.generate to disallow this case?

[mlir][Tensor] Disallow negative dimensions in verifier of tensor.generate

Instead of refusing to transform negative dimensions, the compiler will now crash
which is probably the right thing to do. Thanks to Alex Zinenko for pointing this out.

ftynse requested changes to this revision.May 31 2023, 2:04 AM

Thank you! Please run clang-format.

I'll mark this as "changes requested" so I don't forget to get back to this.

mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
1114

It is usually undesirable to return SmallVector as it may copy a lot. Instead, you can pass a SmallVectorImpl<...> & into the function.

1115

Please declare auxiliary functions as static and document them.

1116

Nit: do not specify the number of stack-allocated vector elements unless there is a strong reason to pick a number, just SmallVector<Value> will do here.

1209–1212

Please expand auto to be the proper type unless the type is obvious from local context (e.g., the RHS has a cast) or difficult/impossible to spell (iterators, lambdas).

This revision now requires changes to proceed.May 31 2023, 2:04 AM

Incorporate reviewer comments

Thanks again for the review Alex. The comments were easy to understand and implement!

ftynse accepted this revision.May 31 2023, 4:35 AM

Thanks again for the patch! Let me know if you need help committing this.

This revision is now accepted and ready to land.May 31 2023, 4:35 AM

Thanks again for the patch! Let me know if you need help committing this.

Yes. :) I don't have permissions.

This revision was automatically updated to reflect the committed changes.