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

Thu Jan 23 09:43:07 PST 2020

ftynse added inline comments.

================
Comment at: mlir/tools/mlir-tblgen/LLVMIRIntrinsicGen.cpp:215
+  os << "def LLVM_" << intr.getProperRecordName() << " : " << opBaseClass
+     << "<\"intr." << intr.getOperationName() << "\", [";
   mlir::interleaveComma(traits, os);
----------------
kariddi wrote:
> ftynse wrote:
> > 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.
> 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)
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.`?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73233