[mlir][Vector] Deprecate vector.extractelement/vector.insertelement

[Groverkss]

See https://discourse.llvm.org/t/rfc-psa-remove-vector-extractelement-and-vector-insertelement-ops-in-favor-of-vector-extract-and-vector-insert-ops/71116/6 for more information.

[Groverkss]

Depends on #113827

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Kunwar Grover (Groverkss)

Changes

See https://discourse.llvm.org/t/rfc-psa-remove-vector-extractelement-and-vector-insertelement-ops-in-favor-of-vector-extract-and-vector-insert-ops/71116/6 for more information.

---

Full diff: https://github.com/llvm/llvm-project/pull/113829.diff

1 Files Affected:

• (modified) mlir/include/mlir/Dialect/Vector/IR/VectorOps.td (+6-2)

diff --git a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
index e859270cf9a5e5..46de656a67054a 100644
--- a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
+++ b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
@@ -635,11 +635,13 @@ def Vector_ExtractElementOp :
     Results<(outs AnyType:$result)> {
   let summary = "extractelement operation";
   let description = [{
+    Note: This operation is depreciated. Please use vector.extract insert.
+
     Takes a 0-D or 1-D vector and a optional dynamic index position and
     extracts the scalar at that position.
 
     Note that this instruction resembles vector.extract, but is restricted to
-    0-D and 1-D vectors and relaxed to dynamic indices.
+    0-D and 1-D vectors.
     If the vector is 0-D, the position must be std::nullopt.
 
 
@@ -819,11 +821,13 @@ def Vector_InsertElementOp :
      Results<(outs AnyVectorOfAnyRank:$result)> {
   let summary = "insertelement operation";
   let description = [{
+    Note: This operation is depreciated. Please use vector.insert instead.
+
     Takes a scalar source, a 0-D or 1-D destination vector and a dynamic index
     position and inserts the source into the destination at the proper position.
 
     Note that this instruction resembles vector.insert, but is restricted to 0-D
-    and 1-D vectors and relaxed to dynamic indices.
+    and 1-D vectors.
 
     It is meant to be closer to LLVM's version:
     https://llvm.org/docs/LangRef.html#insertelement-instruction

[llvmbot]

@llvm/pr-subscribers-mlir-vector

Author: Kunwar Grover (Groverkss)

Changes

See https://discourse.llvm.org/t/rfc-psa-remove-vector-extractelement-and-vector-insertelement-ops-in-favor-of-vector-extract-and-vector-insert-ops/71116/6 for more information.

---

Full diff: https://github.com/llvm/llvm-project/pull/113829.diff

1 Files Affected:

• (modified) mlir/include/mlir/Dialect/Vector/IR/VectorOps.td (+6-2)

diff --git a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
index e859270cf9a5e5..46de656a67054a 100644
--- a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
+++ b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
@@ -635,11 +635,13 @@ def Vector_ExtractElementOp :
     Results<(outs AnyType:$result)> {
   let summary = "extractelement operation";
   let description = [{
+    Note: This operation is depreciated. Please use vector.extract insert.
+
     Takes a 0-D or 1-D vector and a optional dynamic index position and
     extracts the scalar at that position.
 
     Note that this instruction resembles vector.extract, but is restricted to
-    0-D and 1-D vectors and relaxed to dynamic indices.
+    0-D and 1-D vectors.
     If the vector is 0-D, the position must be std::nullopt.
 
 
@@ -819,11 +821,13 @@ def Vector_InsertElementOp :
      Results<(outs AnyVectorOfAnyRank:$result)> {
   let summary = "insertelement operation";
   let description = [{
+    Note: This operation is depreciated. Please use vector.insert instead.
+
     Takes a scalar source, a 0-D or 1-D destination vector and a dynamic index
     position and inserts the source into the destination at the proper position.
 
     Note that this instruction resembles vector.insert, but is restricted to 0-D
-    and 1-D vectors and relaxed to dynamic indices.
+    and 1-D vectors.
 
     It is meant to be closer to LLVM's version:
     https://llvm.org/docs/LangRef.html#insertelement-instruction

[kuhar]

We should also list this on https://mlir.llvm.org/deprecation/

[kuhar]

Before we deprecate this, we should also handle the dynamic case in vector-to-spirv. We support it for insertelement/extractelement but not insert/extract:

• llvm-project/mlir/lib/Conversion/VectorToSPIRV/VectorToSPIRV.cpp

  Lines 252 to 254 in fb33af0

  	int32_t id = getFirstIntValue(insertOp.getMixedPosition());
  	rewriter.replaceOpWithNewOp<spirv::CompositeInsertOp>(
  	insertOp, adaptor.getSource(), adaptor.getDest(), id);

• llvm-project/mlir/lib/Conversion/VectorToSPIRV/VectorToSPIRV.cpp

  Lines 303 to 312 in fb33af0

  	APInt cstPos;
  	if (matchPattern(adaptor.getPosition(), m_ConstantInt(&cstPos)))
  	rewriter.replaceOpWithNewOp<spirv::CompositeInsertOp>(
  	insertOp, adaptor.getSource(), adaptor.getDest(),
  	cstPos.getSExtValue());
  	else
  	rewriter.replaceOpWithNewOp<spirv::VectorInsertDynamicOp>(
  	insertOp, vectorType, insertOp.getDest(), adaptor.getSource(),
  	adaptor.getPosition());
  	return success();

[dcaballe]

LGTM. Final approval should come from @kuhar after addressing his concerns. Thanks!

[Groverkss]

Before we deprecate this, we should also handle the dynamic case in vector-to-spirv. We support it for insertelement/extractelement but not insert/extract:

• llvm-project/mlir/lib/Conversion/VectorToSPIRV/VectorToSPIRV.cpp

  Lines 252 to 254 in fb33af0

  	int32_t id = getFirstIntValue(insertOp.getMixedPosition());
  	rewriter.replaceOpWithNewOp<spirv::CompositeInsertOp>(
  	insertOp, adaptor.getSource(), adaptor.getDest(), id);

• llvm-project/mlir/lib/Conversion/VectorToSPIRV/VectorToSPIRV.cpp

  Lines 303 to 312 in fb33af0

  	APInt cstPos;
  	if (matchPattern(adaptor.getPosition(), m_ConstantInt(&cstPos)))
  	rewriter.replaceOpWithNewOp<spirv::CompositeInsertOp>(
  	insertOp, adaptor.getSource(), adaptor.getDest(),
  	cstPos.getSExtValue());
  	else
  	rewriter.replaceOpWithNewOp<spirv::VectorInsertDynamicOp>(
  	insertOp, vectorType, insertOp.getDest(), adaptor.getSource(),
  	adaptor.getPosition());
  	return success();

#114137

[banach-space]

Hey @Groverkss , how's progress with this? Is this blocked?

[Groverkss]

Hey @Groverkss , how's progress with this? Is this blocked?

Hi! It's not blocked, I just got held up by some other work. I plan to finish sending the final patch to remove all uses of vector.extractelement/vector.insertelement this week.

[banach-space]

Hey @Groverkss , how's progress with this? Is this blocked?

Hi! It's not blocked, I just got held up by some other work. I plan to finish sending the final patch to remove all uses of vector.extractelement/vector.insertelement this week.

Sure, no rush. I've just realised that @kuhar has one outstanding request - I guess that's what we're waiting for? I'd be tempted to land this as is, but will defer to Jakub - I'm happy either way :)

I really appreciate your work in this area 🙏🏻

[kuhar]

The typos from this review are not fixed yet: #113829 (review)