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

Thu Jan 23 09:52:07 PST 2020

kariddi marked an inline comment as done.
kariddi 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);
----------------
ftynse wrote:
> 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.`?
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 :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73233