[PATCH] D74889: [mlir] Generalize intrinsic builders in the LLVM dialect definition

[Aart Bik via Phabricator via llvm-commits]

aartbik added a comment.

I like where this is going a lot. Some suggestions on the doc though



================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOpBase.td:59
 
-// 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.*".
+// Deprecated compatibility class for LLVM intrinsic operations.
 class LLVM_IntrOp<string mnemonic, list<OpTrait> traits = []> :
----------------
If this is deprecated, add a TODO on the plan on the path forward (e.g. remove when ....)


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOpBase.td:106
+// of operation. "$0" is expected to be replaced by the position of the operand
+// or result in the operaiton.
+def LLVM_IntrPatterns {
----------------
typo: operation


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOpBase.td:121
+// that contain the positions of intrinsic results and operands that are
+// overloadable in LLVM sense, that is their types must be passed in during the
+// construction of the intrinsic declaration to differentiate between
----------------
in *the* LLVM sense


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOpBase.td:148
+// Base class for LLVM intrinsic operations returning no results. Places the
+// intrinsic into the LLVM dialect and prefixes its name with "intr.".
+class LLVM_ZeroResultIntrOp<string mnem, list<int> overloadedResults = [],
----------------
since this (and L157) is the class doc most people will read (and not the doc of the implementation oriented base class), some more doc on sample use would be useful.

E.g.
sample use:
      LLVM_OneResultIntrOp<"name",  [0], [1], []>,   Arguments<(ins LLVM_Type, LLVM_Type)>;
means
 ....


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74889