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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:789
+    Arguments<(ins LLVM_Type)>, Results<(outs LLVM_Type:$res)> {
+  let llvmBuilder = [{
+    llvm::Module *module = builder.GetInsertBlock()->getModule();
----------------
aartbik wrote:
> rriddle wrote:
> > Can you define a template for these? The builder is effectively the same for all of these.
> Ha! I actually started to write a template by hand and then was pointed to Alex' brilliant tool to generate this code automatically. I am okay cleaning up the code by hand now, but then some of the automation is lost :-)
We should eventually injecting the generated code into the file instead of copy-pasting, but for now it's fine if it stays as is.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:795
+        .getUnderlyingType(),
+  });
+    auto operands =
----------------
aartbik wrote:
> 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?
It's an open project to format those :)


================
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:
> 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 ;-)
@nicolasvasilache  this test does exactly `mlir-translate --mlir-to-llvmir`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74171