[PATCH] D80842: [mlir] Add verify method to adaptor

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar marked an inline comment as done.
jpienaar added a comment.

In D80842#2070603 <https://reviews.llvm.org/D80842#2070603>, @ftynse wrote:

> I would consider renaming OperandAdaptor to just OpAdaptor because it becomes concerned with more than operands.


Agreed, but will do as purely mechanical change.



================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:333
+                 formatv("{0}(\"{1}\")", attrGet, namedAttr.name));
+  for (int i = 0, e = op.getNumOperands(); i < e; ++i) {
+    auto &value = op.getOperand(i);
----------------
aartbik wrote:
> nit: here and below, "unsigned int" seems slightly more idiomatic
getNumOperands is a signed int type, I prefer not to mix signed/unsigned computations. 

(Disclaimer: I also support http://lists.llvm.org/pipermail/llvm-dev/2019-June/132890.html and would prefer we consistently used ints outside bitmasks, but thats a different discussion).


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:394
+
+    if (!emitVerificationRequiringOp && !allowMissingAttr) {
+      body << "  if (!" << varName << ") return " << emitErrorPrefix
----------------
ftynse wrote:
> This looks like we don't allow optional/default-valued attributes with conditions that require the op to be constructed. I'm not sure that's right.
We do allow, we just don't verify it in the adaptor as there is nothing we can verify (op may not exist yet and attribute need not be defined),  it would/should be verified in the op's verify method.


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:1622
 
+  genAttributeVerifier(op, "this->getAttr", "emitOpError(",
+                       /*emitVerificationRequiringOp=*/true, verifyCtx, body);
----------------
ftynse wrote:
> We should document somewhere that one cannot assume anything about the order in which auto-generated attribute verifier get applied
This is true today yes (although it is in order at present, that is an implementation detail rather than contract). We do actually want to enable an order but that is a different discussion.

[added to md doc]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80842