[Mlir-commits] [mlir] [mlir][vector] Add verification for incorrect vector.extract (PR #115824)

[Diego Caballero]

================
@@ -1339,6 +1339,50 @@ bool ExtractOp::isCompatibleReturnTypes(TypeRange l, TypeRange r) {
   return l == r;
 }
 
+// Common verification rules for `InsertOp` and `ExtractOp` involving indices.
+// `indexedType` is the vector type being indexed in the operation, i.e., the
+// destination type in InsertOp and the source type in ExtractOp.
+// `vecOrScalarType` is the type that is not indexed in the op and can be
+// either a scalar or a vector, i.e., the source type in InsertOp and the
+// return type in ExtractOp.
+static LogicalResult verifyInsertExtractIndices(Operation *op,
+                                                VectorType indexedType,
+                                                int64_t numIndices,
+                                                Type vecOrScalarType) {
+  int64_t indexedRank = indexedType.getRank();
+  if (numIndices > indexedRank)
+    return op->emitOpError(
+        "expected a number of indices no greater than the indexed vector rank");
+
+  if (auto nonIndexedVecType = dyn_cast<VectorType>(vecOrScalarType)) {
+    // Vector case, including:
+    //  * 0-D vector:
+    //    * vector.extract %src[2]: vector<f32> from vector<8xf32)
----------------
dcaballe wrote:

Thanks for the analysis! Replying quickly for now.

> Degenerates to an element type if n-k is zero.

That was indeed the case. I believe the introduction of the "isCompatibleType" logic accidentally broke that rule. Honestly, I wouldn't mind going back to that state.

For me, the guiding principle here is that the current implementation is too loose. By making it stricter, even if too much, intentionally, we can have discussions on actual use cases on a case by case basis. Otherwise, we might spend a lot of time arguing about scenarios that may not be practical. One thing I really want to avoid is allowing cases that lead to the introduction of canonicalization patterns to turn `vector.extract/insert` into `vector.bitcast`. That would open these operations to a new level of ambiguity.

Please, let me know what you think and I'll revisit the logic and the valid/invalid cases for final alignment. 

https://github.com/llvm/llvm-project/pull/115824