[mlir][vector] Add build method for vector.to_elements

[newling]

The default builder (added in 7aecd7e) requires result types:

build(builder, result, result_types, elements);

but these types are uniquely defined by the input vector elements. This PR adds a builder to do this mechanical type inference.

Testing: not obvious how to unit test this, I can wait until this PR can be stacked with something that can use it, if that's preferred

[llvmbot]

@llvm/pr-subscribers-mlir-vector

@llvm/pr-subscribers-mlir

Author: James Newling (newling)

Changes

The default builder (added in 7aecd7e) requires result types:

build(builder, result, result_types, elements);

but these types are uniquely defined by the input vector elements. This PR adds a builder to do this mechanical type inference.

Testing: not obvious how to unit test this, I can wait until this PR can be stacked with something that can use it, if that's preferred

---

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

2 Files Affected:

• (modified) mlir/include/mlir/Dialect/Vector/IR/VectorOps.td (+9-2)
• (modified) mlir/lib/Dialect/Vector/IR/VectorOps.cpp (+8)

diff --git a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
index aef156c5f1d05..fc99a8e30ef1f 100644
--- a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
+++ b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
@@ -798,7 +798,7 @@ def Vector_ToElementsOp : Vector_Op<"to_elements", [
     This operation decomposes all the scalar elements from a vector. The
     decomposed scalar elements are returned in row-major order. The number of
     scalar results must match the number of elements in the input vector type.
-    All the result elements have the same result type, which must match the
+    All the result elements have the same type, which must match the
     element type of the input vector. Scalable vectors are not supported.
 
     Examples:
@@ -813,7 +813,7 @@ def Vector_ToElementsOp : Vector_Op<"to_elements", [
     // %0#0 = %v1[0]
     // %0#1 = %v1[1]
 
-    // Decompose a 2-D.
+    // Decompose a 2-D vector.
     %0:6 = vector.to_elements %v2 : vector<2x3xf32>
     // %0#0 = %v2[0, 0]
     // %0#1 = %v2[0, 1]
@@ -835,6 +835,13 @@ def Vector_ToElementsOp : Vector_Op<"to_elements", [
 
   let arguments = (ins AnyVectorOfAnyRank:$source);
   let results = (outs Variadic<AnyType>:$elements);
+
+
+  let builders = [
+    // Build method that infers the result types from `elements`.
+    OpBuilder<(ins "Value":$elements)>,
+  ];
+
   let assemblyFormat = "$source attr-dict `:` type($source)";
   let hasFolder = 1;
 }
diff --git a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
index 6f0ac6bb58282..a2921d3c89d14 100644
--- a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
+++ b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
@@ -2417,6 +2417,14 @@ LogicalResult ToElementsOp::fold(FoldAdaptor adaptor,
   return foldToElementsFromElements(*this, results);
 }
 
+void vector::ToElementsOp::build(OpBuilder &builder, OperationState &result, Value elements) {
+  auto vectorType = cast<VectorType>(elements.getType());
+  Type elementType = vectorType.getElementType();
+  int64_t nbElements = vectorType.getNumElements();
+  SmallVector<Type> scalarTypes(nbElements, elementType);
+  build(builder, result, scalarTypes, elements);
+}
+
 //===----------------------------------------------------------------------===//
 // FromElementsOp
 //===----------------------------------------------------------------------===//

[dcaballe]

Thanks for helping!

[dcaballe]

nb?

[joker-eph]

Suggested change

	int64_t nbElements = vectorType.getNumElements();
	int64_t numElements = vectorType.getNumElements();

[banach-space]

Thanks @newling !

Testing: not obvious how to unit test this, I can wait until this PR can be stacked with something that can use it, if that's preferred

We should find some use for this code first. How about removing the current builder build(builder, result, result_types, elements); and using the new builder instead? This way we will know that the new builder is used + tested.

[newling]

Thanks @newling !

Testing: not obvious how to unit test this, I can wait until this PR can be stacked with something that can use it, if that's preferred

We should find some use for this code first. How about removing the current builder build(builder, result, result_types, elements); and using the new builder instead? This way we will know that the new builder is used + tested.

That'd be great, but is there a way to delete this default build method?

[joker-eph]

If you delete it, you can't use it in the implementation of the builder like you're doing here.

[dcaballe]

If you delete it, you can't use it in the implementation of the builder like you're doing here.

Yeah, and parser would fail as well?

I think we should land this. This is going to be used upstream once we start generating these ops from existing patterns but we need a bit of time to fill up all the lowering a canonicalization gaps. In the meantime, we can use this build for downstream integration and experimentation, which I guess it's what James and I are doing right now.

[banach-space]

Folks, I'm not comfortable accepting this PR in its current state.

I can wait until this PR can be stacked with something that can use it, if that's preferred

This would be my preference.

While I have nothing against the change in principle, without tests or any upstream usage, it currently looks like "dead code." That means it's vulnerable to being removed later on, and it's hard to justify exceptions - how would we distinguish it from other unused code upstream?

In the meantime, we can use this build for downstream integration and experimentation

My main concern is the precedent this sets. Landing code without tests or usage, even with good intentions, weakens our standards and makes future exceptions harder to argue against. It's important we maintain a consistent bar for what's accepted upstream.

That said, I'm not blocking the PR - just expressing a strong preference to hold off until this can land alongside its first user.

[newling]

I completely agree @banach-space.

My stack that uses it is probably still a few weeks out (apologies for posting this PR prematurely), so @dcaballe please feel free to replicate/cherry this onto any work you have that might use it.