[PATCH] D142445: [mlir][tensor|memref] Harden the checks on dim op

[Aart Bik via Phabricator via llvm-commits]

aartbik accepted this revision.
aartbik added inline comments.
This revision is now accepted and ready to land.


================
Comment at: mlir/include/mlir/IR/OpBase.td:757-766
+class Non0RankedTensorOf<list<Type> allowedTypes>
+  : TensorOf<allowedTypes, [HasRankGreaterOrEqualPred<1>],
+      "non-0-ranked.tensor">;
+
 def AnyRankedTensor : RankedTensorOf<[AnyType]>;
+def AnyNon0RankedTensor  : Non0RankedTensorOf<[AnyType]>;
+def AnyUnrankedTensor  : UnrankedTensorOf<[AnyType]>;
----------------
qcolombet wrote:
> springerm wrote:
> > Side note: Adding a manual check to the op verifier would just be two lines. Are there any other ops that could benefit from `Non0RankedTensorOf` etc.?
> Not that I know of on top of my head.
> 
> For some reasons, I feel that having the check in the td file is easier to discover (the "documentation" is all in one place), but I'm happy to move it to pure C++ in the verifier if that's what people prefer.
Isn't the declarative way always preferred over the imperative C++ way? This looks just fine to me ;-)


================
Comment at: mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp:957
   if (auto memrefType = type.dyn_cast<MemRefType>()) {
-    if (*index >= memrefType.getRank())
+    if (uindex >= (uint64_t)memrefType.getRank())
       return emitOpError("index is out of range");
----------------
perhaps assign this to

uint64_t rank = ....

and then compare for readability


================
Comment at: mlir/test/Dialect/MemRef/invalid.mlir:1064
+func.func @dim_negative_idx(%arg : memref<f32>, %arg1 : index) {
+  memref.dim %arg, %arg1 : memref<f32> // expected-error {{'memref.dim' op operand #0 must be unranked.memref of any type values or non-0-ranked.memref of any type values, but got 'memref<f32>'}}
+  return
----------------
yeah, thanks for rejecting this!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142445/new/

https://reviews.llvm.org/D142445