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

[kariddi]

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.

[merge_guards_bot]

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]

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.

[kariddi]

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]

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]

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]

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]

Thanks!

[merge_guards_bot]

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.

mlir/include/mlir/Dialect/LLVMIR/LLVMOpBase.td

 
 // Base class for LLVM operations. All operations get an "llvm." prefix in
 // their name automatically. LLVM operations have either zero or one result,
 // this class is specialized below for both cases and should not be used
 // directly.
 class LLVM_Op<string mnemonic, list<OpTrait> traits = []> :
     LLVM_OpBase<LLVM_Dialect, mnemonic, traits>;
 
+// Base class for LLVM intrinsics operation. It is the same as an LLVM_Op
+// but the operation has a ".intr." element in the prefix becoming
+// "llvm.intr.*".
+class LLVM_IntrOp<string mnemonic, list<OpTrait> traits = []> :
+    LLVM_Op<"intr."#mnemonic, traits>;
+
 #endif  // LLVMIR_OP_BASE

mlir/test/mlir-tblgen/llvm-intrinsics.td

 // This checks that we can consume LLVM's Intrinsic definitions from TableGen
 // files and produce ODS. Unlike MLIR, LLVM's main Intrinsics.td file that
 // contains the definition of the Intrinsic class also includes files for
 // platform-specific intrinsics, so we need to give it to TableGen instead of
 // writing a local test source. We filter out platform-specific intrinsic
 // includes from the main file to avoid unnecessary dependencies and decrease
 // the test cost. The command-line flags further ensure a specific intrinsic is
 // processed and we only check the ouptut below.
 // We also verify emission of type specialization for overloadable intrinsics.
 //
 // RUN: cat %S/../../../llvm/include/llvm/IR/Intrinsics.td \
 // RUN: | grep -v "llvm/IR/Intrinsics" \
-// RUN: | mlir-tblgen -gen-llvmir-intrinsics -I %S/../../../llvm/include/ --llvmir-intrinsics-filter=is_constant \
+// RUN: | mlir-tblgen -gen-llvmir-intrinsics -I %S/../../../llvm/include/ --llvmir-intrinsics-filter=ptrmask \
 // RUN: | FileCheck %s
 
-// CHECK-LABEL: def LLVM_is_constant
-// CHECK: LLVM_Op<"intr
+// CHECK-LABEL: def LLVM_ptrmask
+// CHECK: LLVM_IntrOp<"ptrmask
 // CHECK: Arguments<(ins
 // CHECK: Results<(outs
 // CHECK: llvm::Function *fn = llvm::Intrinsic::getDeclaration(
-// CHECK:        module, llvm::Intrinsic::is_constant, {
+// CHECK:        module, llvm::Intrinsic::ptrmask, {
+// CHECK:        opInst.getResult(0).getType().cast<LLVM::LLVMType>().getUnderlyingType(),
 // CHECK:        opInst.getOperand(0).getType().cast<LLVM::LLVMType>().getUnderlyingType(),
+// CHECK:        opInst.getOperand(1).getType().cast<LLVM::LLVMType>().getUnderlyingType(),
 // CHECK: });
 // CHECK:        lookupValues(opInst.getOperands());
 
 //---------------------------------------------------------------------------//
 
 // This checks that the ODS we produce can be consumed by MLIR tablegen. We only
 // make sure the entire process does not fail and produces some C++. The shape
 // of this C++ code is tested by ODS tests.
 
 // RUN: cat %S/../../../llvm/include/llvm/IR/Intrinsics.td \
 // RUN: | grep -v "llvm/IR/Intrinsics" \
 // RUN: | mlir-tblgen -gen-llvmir-intrinsics -I %S/../../../llvm/include/ --llvmir-intrinsics-filter=vastart \
 // RUN: | mlir-tblgen -gen-op-decls -I %S/../../include \
 // RUN: | FileCheck --check-prefix=ODS %s
 
 // ODS-LABEL: class vastart
+
+// RUN: cat %S/../../../llvm/include/llvm/IR/Intrinsics.td \
+// RUN: | grep -v "llvm/IR/Intrinsics" \
+// RUN: | mlir-tblgen -gen-llvmir-intrinsics -I %S/../../../llvm/include/ --llvmir-intrinsics-filter=ptrmask -dialect-opclass-base My_OpBase \
+// RUN: | FileCheck %s --check-prefix=DIALECT-OPBASE
+
+// DIALECT-OPBASE-LABEL: def LLVM_ptrmask
+// DIALECT-OPBASE: My_OpBase<"ptrmask

mlir/tools/mlir-tblgen/LLVMIRIntrinsicGen.cpp

 static llvm::cl::OptionCategory IntrinsicGenCat("Intrinsics Generator Options");
 
 static llvm::cl::opt<std::string>
     nameFilter("llvmir-intrinsics-filter",
                llvm::cl::desc("Only keep the intrinsics with the specified "
                               "substring in their record name"),
                llvm::cl::cat(IntrinsicGenCat));
 
+static llvm::cl::opt<std::string>
+    opBaseClass("dialect-opclass-base",
+                llvm::cl::desc("The base class for the ops in the dialect we "
+                               "are planning to emit"),
+                llvm::cl::init("LLVM_IntrOp"), llvm::cl::cat(IntrinsicGenCat));
+
 // Used to represent the indices of overloadable operands/results.
 using IndicesTy = llvm::SmallBitVector;
 
 /// Return a CodeGen value type entry from a type record.
 static llvm::MVT::SimpleValueType getValueType(const llvm::Record *rec) {
   return (llvm::MVT::SimpleValueType)rec->getValueAsDef("VT")->getValueAsInt(
       "Value");
 }
std::string getOperationName() const {
     if (!name.empty())
       return name.str();
 
     name = record.getName();
     assert(name.startswith("int_") &&
            "LLVM intrinsic names are expected to start with 'int_'");
     name = name.drop_front(4);
     llvm::SmallVector<llvm::StringRef, 8> chunks;
+    llvm::StringRef targetPrefix = record.getValueAsString("TargetPrefix");
     name.split(chunks, '_');
-    return llvm::join(chunks, ".");
+    auto chunksBegin = chunks.begin();
+    // Remove the target prefix from target specific intrinsics.
+    if (!targetPrefix.empty()) {
+      assert(targetPrefix == *chunksBegin &&
+             "Intrinsic has TargetPrefix, but "
+             "record name doesn't begin with it");
+      assert(chunks.size() >= 2 &&
+             "Intrinsic has TargetPrefix, but "
+             "chunks has only one element meaning the intrinsic name is empty");
+      ++chunksBegin;
+    }
+    return llvm::join(chunksBegin, chunks.end(), ".");
   }
 
   /// Get the name of the record without the "intrinsic" prefix.
   llvm::StringRef getProperRecordName() const {
     llvm::StringRef name = record.getName();
     assert(name.startswith("int_") &&
            "LLVM intrinsic names are expected to start with 'int_'");
     return name.drop_front(4);
static bool emitIntrinsic(const llvm::Record &record, llvm::raw_ostream &os) {
   if (!intr.hasSideEffects())
     traits.push_back("NoSideEffect");
 
   // Prepare strings for operands.
   llvm::SmallVector<llvm::StringRef, 8> operands(intr.getNumOperands(),
                                                  "LLVM_Type");
 
   // Emit the definition.
-  os << "def LLVM_" << intr.getProperRecordName() << " : LLVM_Op<\"intr."
-     << intr.getOperationName() << "\", [";
+  os << "def LLVM_" << intr.getProperRecordName() << " : " << opBaseClass
+     << "<\"" << intr.getOperationName() << "\", [";
   mlir::interleaveComma(traits, os);
   os << "]>, Arguments<(ins" << (operands.empty() ? "" : " ");
   mlir::interleaveComma(operands, os);
   os << ")>, Results<(outs"
      << (intr.getNumResults() == 0 ? "" : " LLVM_Type:$res") << ")> {\n"
      << "  let llvmBuilder = [{\n";
   emitBuilder(intr, os);
   os << "}];\n";