[Mlir-commits] [mlir] [mlir] Return vectorized values instead of replacing (PR #144158)

[llvmlistbot at llvm.org]

================
@@ -771,17 +771,24 @@ LogicalResult deallocateGPUPrivateMemory(OpBuilder &, Value /*buffer*/);
 /// to work (these are checked by the vectorizer itself).
 bool hasVectorizationImpl(Operation *);
 
+/// Transformation information returned after vectorizing.
+struct VectorizationResult {
+  /// Results of the vectorization transform to replace the original operation.
+  SmallVector<Value> replacements;
+};
----------------
Max191 wrote:

> Why not:
  ```using vectorizedValues = SmallVector<Value>;```
?

I made it a struct because it leaves more room for extending the return type with less code churn in downstream projects, although I don't have a strong opinion on this.

> Also, the newly introduced/renamed VectorizationHookResult is used more widely than VectorizationResult - I would optimise for what's more commonly used and just keep the old name

IMO, the naming of this new return type is more important to get right, because the `VectorizationHookResult` is just an internal implementation detail to the file. The new `VectorizationResult` is exposed as the return type of the public vectorize function, which means there will be more pain in downstream projects any time the type name is changed. A change to `VectorizationHookResult` is done with a single PR here in mlir, but changing `VectorizationResult` means every downstream project must integrate the change as well. Again, I don't have a particularly strong opinion here, but it just seems nice to avoid potential extra integration pain downstream.

That said, comments to distinguish the two are always great! I'll try to clarify it in the code comments.

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