[PATCH] D74171: [mlir] [LLVMIR] add all vector reduction intrinsics to LLVM IR dialect

[Aart Bik via Phabricator via llvm-commits]

aartbik marked 5 inline comments as done.
aartbik added inline comments.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:795
+        .getUnderlyingType(),
+  });
+    auto operands =
----------------
ftynse wrote:
> Something went wrong here with braces, mismerge?
I took the code as generated by your awesome utility, and only reformatted on the 80-col violation. This bracket was placed here.

Is there any way to format a *.td file automatically?


================
Comment at: mlir/test/Target/llvmir-intrinsics.mlir:98
 
+// CHECK-LABEL: @vector_reductions
+llvm.func @vector_reductions(%arg0: !llvm.float, %arg1: !llvm<"<8 x float>">, %arg2: !llvm<"<8 x i32>">) {
----------------
nicolasvasilache wrote:
> nicolasvasilache wrote:
> > Did you try to pipe these through `mlir-translate --mlir-to-llvmir` ?
> > I've had surprises on my end with more complex masked operations.
> I would also argue that we should have an extra test in this file that does the piping through `mlir-translate --mlir-to-llvmir` (no need to filecheck the result just that it doesn't crash).
> However @ftynse repeatedly made the case against it.
> But I don't buy the separability of tests argument here :) esp in light of how easy it is to get crashes when trying to go all the way down to actual LLVMIR.
Yes Sir! I typically tests my examples with mlir-opt and mlir-translate to be sure. They only time I saw a crash was using an integer intrinsic on a floating-point value by mistake (something we may want to more gracefully detect at some point).


================
Comment at: mlir/test/Target/llvmir-intrinsics.mlir:98
 
+// CHECK-LABEL: @vector_reductions
+llvm.func @vector_reductions(%arg0: !llvm.float, %arg1: !llvm<"<8 x float>">, %arg2: !llvm<"<8 x i32>">) {
----------------
aartbik wrote:
> nicolasvasilache wrote:
> > nicolasvasilache wrote:
> > > Did you try to pipe these through `mlir-translate --mlir-to-llvmir` ?
> > > I've had surprises on my end with more complex masked operations.
> > I would also argue that we should have an extra test in this file that does the piping through `mlir-translate --mlir-to-llvmir` (no need to filecheck the result just that it doesn't crash).
> > However @ftynse repeatedly made the case against it.
> > But I don't buy the separability of tests argument here :) esp in light of how easy it is to get crashes when trying to go all the way down to actual LLVMIR.
> Yes Sir! I typically tests my examples with mlir-opt and mlir-translate to be sure. They only time I saw a crash was using an integer intrinsic on a floating-point value by mistake (something we may want to more gracefully detect at some point).
Ack on the test, but I am guessing not in this CL ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74171