Page MenuHomePhabricator

[mlir] [LLVMIR] add all vector reduction intrinsics to LLVM IR dialect
ClosedPublic

Authored by aartbik on Thu, Feb 6, 2:22 PM.

Details

Summary

This allows for lowering of VectorOps (and others) into a LLVM IR
that maps directly to efficient implementations on the target machines.

http://llvm.org/docs/LangRef.html#experimental-vector-reduction-intrinsics

Diff Detail

Event Timeline

aartbik created this revision.Thu, Feb 6, 2:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Feb 6, 2:22 PM
ftynse requested changes to this revision.Fri, Feb 7, 4:35 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
796

Something went wrong here with braces, mismerge?

This revision now requires changes to proceed.Fri, Feb 7, 4:35 AM
mlir/test/Target/llvmir-intrinsics.mlir
102

Did you try to pipe these through mlir-translate --mlir-to-llvmir ?
I've had surprises on my end with more complex masked operations.

mlir/test/Target/llvmir-intrinsics.mlir
102

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.

rriddle requested changes to this revision.Fri, Feb 7, 10:24 AM
rriddle added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
790

Can you define a template for these? The builder is effectively the same for all of these.

aartbik marked 5 inline comments as done.Fri, Feb 7, 10:25 AM
aartbik added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
796

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?

mlir/test/Target/llvmir-intrinsics.mlir
102

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).

102

Ack on the test, but I am guessing not in this CL ;-)

aartbik marked 2 inline comments as done.Fri, Feb 7, 10:33 AM
aartbik added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
790

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 :-)

ftynse accepted this revision.Fri, Feb 7, 11:10 AM
ftynse added inline comments.Fri, Feb 7, 11:13 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
790

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.

796

It's an open project to format those :)

mlir/test/Target/llvmir-intrinsics.mlir
102

@nicolasvasilache this test does exactly mlir-translate --mlir-to-llvmir

rriddle added inline comments.Fri, Feb 7, 11:24 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
790

So the plan is to have a GeneratedOps.td alongside this one? That makes sense if so, but can we start a new file for generated ops prematurely to avoid extra work later on? It also keeps this file clean so that people don't look at it as just technical debt laying around to be cleaned up.

ftynse added inline comments.Fri, Feb 7, 1:35 PM
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
790

Good idea, let's have a separate file now and clearly mark it as autogenerated to avoid random edits. The generator should already produce a header explaining that.

aartbik updated this revision to Diff 243288.Fri, Feb 7, 2:33 PM

less code is better

Okay, friends of concise code, PTAL

rriddle accepted this revision.Sun, Feb 9, 10:45 PM
This revision is now accepted and ready to land.Sun, Feb 9, 10:45 PM
ftynse accepted this revision.Mon, Feb 10, 8:28 AM
This revision was automatically updated to reflect the committed changes.