Page MenuHomePhabricator

[mlir] Add option to use custom base class for dialect in LLVMIRIntrinsicGen.
ClosedPublic

Authored by kariddi on Jan 22 2020, 2:31 PM.

Details

Summary

LLVMIRIntrinsicGen is using LLVM_Op as the base class for intrinsics.
This works for LLVM intrinsics in the LLVM Dialect, but when we are
trying to convert custom intrinsics that originate from a custom
LLVM dialect (like NVVM or ROCDL) these usually have a different
"cppNamespace" that needs to be applied to these dialect.

These dialect specific characteristics (like "cppNamespace")
are typically organized by creating a custom op (like NVVM_Op or
ROCDL_Op) that passes the correct dialect to the LLVM_OpBase class.

It seems natural to allow LLVMIRIntrinsicGen to take that into
consideration when generating the conversion code from one of these
dialect to a set of target specific intrinsics.

Diff Detail

Event Timeline

kariddi created this revision.Jan 22 2020, 2:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2020, 2:31 PM

Unit tests: pass. 62114 tests passed, 0 failed and 808 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

ftynse requested changes to this revision.Jan 23 2020, 7:23 AM

Looks good in general, I am suggesting to push it one step further.

mlir/tools/mlir-tblgen/LLVMIRIntrinsicGen.cpp
227

I wold consider dropping the "intr." part here, and rather defining a new LLVM_IntrOp that derives LLVM_Op in Tablegen and using your new functionality to obtain llvm.intr.* names. Other sets of intrinsics don't necessarily follow this naming convention (e.g. we have nvvm.barrier0, not nvvm.intr.barrier0 and the intrinsic/operation makes sense only for the "core" LLVM parts.

This revision now requires changes to proceed.Jan 23 2020, 7:23 AM
kariddi marked an inline comment as done.Jan 23 2020, 9:28 AM
kariddi added inline comments.
mlir/tools/mlir-tblgen/LLVMIRIntrinsicGen.cpp
227

Hi Alex, thanks for the comment, I agree, that's a good idea! I'm almost done with it, but I noticed that the strings produced actually look like "llvm.intr.llvm.is.constant" or "nvvm.intr.nvvm.barrier0"

For example this is the current code generated:
LLVM_Op<"intr.nvvm.barrier0", ...

Do you think we should drop the nvvm after the "intr." as well (as it is redundant with respect to the dialect usually)

ftynse added inline comments.Jan 23 2020, 9:41 AM
mlir/tools/mlir-tblgen/LLVMIRIntrinsicGen.cpp
227

I'd keep only nvvm.barrier0, we already have that one actually https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/Dialect/LLVMIR/NVVMOps.td#L76 so it will be more consistent. From MLIR perspective, it makes sense to treat NVVM as a dialect, and other targets-specific intrinsics as separate dialects as well.

The llvm.intr.llvm.is.constant is unfortunate. Can we filter out the first llvm.?

kariddi marked an inline comment as done.Jan 23 2020, 9:51 AM
kariddi added inline comments.
mlir/tools/mlir-tblgen/LLVMIRIntrinsicGen.cpp
227

I think I can filter out the architecture (which would be "llvm." for LLVM intrinsics and on target specific intrinsics would be "aarch64." or "nvvm." for example

I'll quickly make an update and then you can tell me if I'm going in the right direction :)

kariddi updated this revision to Diff 239939.Jan 23 2020, 10:35 AM

Ok, in this patch I'm stripping away the "TargetPrefix" off the intrinsics.
I don't know if there are some users that might want to keep that (at which point we might add it as a command line option)
but this avoids the nvvm.intr.nvvm.barrier0 problem, making the output just nvvm.intr.barrier0

An example of the new output here:

def LLVM_nvvm_barrier0 : NVVM_Op<"barrier0", [NoSideEffect]>, Arguments<(ins)>, Results<(outs)> {
...
}

I couldn't solve the problem of llvm.is.constant though, because this instrinsic is special ...
It actually sets the "name" field in the Intrinsic<> definition, which just a handful of intrinsic
do.

I don't feel is safe to try to strip elements off user defined names, so I preferred to keep it untouched.

I changed the test to use another intrinsic instead of is_constant that is more representative.
(as it doesn't use the "name" field like most of the intrinsics and has both overloadable result and operands.

ftynse accepted this revision.Jan 23 2020, 10:58 AM

Thanks!

This revision is now accepted and ready to land.Jan 23 2020, 10:58 AM
This revision was automatically updated to reflect the committed changes.

Unit tests: fail. 62131 tests passed, 5 failed and 811 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.