[PATCH] D74075: [mlir][VectorOps] Introduce a `vector.fma` op that works on n-D vectors and lowers to `llvm.intrin.fmuladd`

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added a comment.

Thanks, Nicolas! LGTM. Just minor changes.



================
Comment at: mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp:413
 
+class VectorFMAOpConversion : public LLVMOpLowering {
+public:
----------------
Adding doc about what this pattern is matching and what is not would be great


================
Comment at: mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp:526
 
+// Rank reducing rewrite for n-D FMA into (n-1)-D FMA where n > 1.
+class VectorFMAOpRewritePattern : public OpRewritePattern<FMAOp> {
----------------
I think adding a small example to the doc would help better understand what this patter is doing.


================
Comment at: mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp:1013
                   VectorExtractElementOpConversion, VectorExtractOpConversion,
-                  VectorInsertElementOpConversion, VectorInsertOpConversion,
-                  VectorOuterProductOpConversion, VectorTypeCastOpConversion,
-                  VectorPrintOpConversion>(ctx, converter);
+                  VectorFMAOpConversion, VectorInsertElementOpConversion,
+                  VectorInsertOpConversion, VectorOuterProductOpConversion,
----------------
Rename `VectorFMAOpRewritePattern` and `VectorFMAOpConversion` to be more aligned with what they match?
You'll probably find something better but something along the lines of `SingleDimVectorFMAOpRewritePattern` and `MultiDimVectorFMAOpConversion`


================
Comment at: mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir:618
+{
+  //         CHECK: llvm.intr.fmuladd{{.*}}: (!llvm<"<8 x float>">, !llvm<"<8 x float>">, !llvm<"<8 x float>">) -> !llvm<"<8 x float>">
+  %0 = vector.fma %a, %a, %a : vector<8xf32>
----------------
aartbik wrote:
> perhaps use the  %[[s0:.*]]  = .... to capture the values and make sure they are used where expected?
+1


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74075/new/

https://reviews.llvm.org/D74075