[Mlir-commits] [mlir] [mlir][Vector] Fix vector.extract lowering to llvm for 0-d vectors (PR #117731)

[Andrzej Warzyński]

================
@@ -1096,43 +1096,56 @@ class VectorExtractOpConversion
     SmallVector<OpFoldResult> positionVec = getMixedValues(
         adaptor.getStaticPosition(), adaptor.getDynamicPosition(), rewriter);
 
-    // Extract entire vector. Should be handled by folder, but just to be safe.
-    ArrayRef<OpFoldResult> position(positionVec);
-    if (position.empty()) {
-      rewriter.replaceOp(extractOp, adaptor.getVector());
-      return success();
-    }
-
-    // One-shot extraction of vector from array (only requires extractvalue).
-    // Except for extracting 1-element vectors.
-    if (isa<VectorType>(resultType) &&
-        position.size() !=
-            static_cast<size_t>(extractOp.getSourceVectorType().getRank())) {
-      if (extractOp.hasDynamicPosition())
-        return failure();
-
-      Value extracted = rewriter.create<LLVM::ExtractValueOp>(
-          loc, adaptor.getVector(), getAsIntegers(position));
-      rewriter.replaceOp(extractOp, extracted);
-      return success();
-    }
+    // The LLVM lowering models multi dimension vectors as stacked 1-d vectors.
+    // The stacking is modeled using arrays. We do this conversion from a
+    // N-d vector extract to stacked 1-d vector extract in two steps:
+    //  - Extract a 1-d vector or a stack of 1-d vectors (llvm.extractvalue)
+    //  - Extract a scalar out of the 1-d vector if needed (llvm.extractelement)
+
+    // Determine if we need to extract a slice out of the original vector. We
+    // always need to extract a slice if the input rank >= 2.
+    bool isSlicingExtract = extractOp.getSourceVectorType().getRank() >= 2;
----------------
banach-space wrote:

At the LLVM level, we don't truly "extract slices"—instead, we extract a member from an aggregate. For reference:

* [LLVM LangRef: Aggregate Operations](https://llvm.org/docs/LangRef.html#aggregate-operations)

According to the LangRef:

* `extractelement` extracts a scalar.
* `extractvalue` extracts a value, which in this case could be an aggregate or a vector.

This duality is particularly confusing because "Extract" exists at both the `Vector` and `LLVM` levels. To address this, perhaps we could rename the predicates for clarity:

* Rename `isSlicingExtract` -> `extractsAggregate` or `extractsValue`.
* Rename `isScalarExtract` -> `extractsScalar` or `extractsElement`.

Naming is hard 🤷🏻‍♂️. My suggestion would be to aim for terms that:

* Align closely with existing taxonomy/terminology.
* Clearly distinguish whether "extract" refers to `Vector` or `LLVM` (i.e., pre-lowering vs. post-lowering extraction).

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