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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added a comment.
This revision now requires changes to proceed.
Herald added a project: MLIR.

Thanks! I think this is useful, and may also be helpful for some parsers that rely on specific attributes being present to interpret the syntax.

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



================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:324
 
+// Populate the format context for substitutions of attributes, operands and
+// results.
----------------
aartbik wrote:
> .. format context 'ctx'  for
> (since this is an output var, good to document)
In general, a little description of what is expected in attrGet/OperandGet/resultGet would be helpful here. I see from the code that they are used as function-like objects that are called with an integer literal and should return something (presumably a range) that I don't fully grasp.


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:361
+// generated.
+static void genAttributeVerifier(const Operator &op, const char *attrGet,
+                                 const char *emitErrorPrefix,
----------------
Same as above, could you please add a doc on what is the expected format of `emitErrorPrefix` (I suppose it should be a quoted string literal with a trailing space)


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:377
+    bool hasConditionToEmit =
+        !attrPred.isNull() && (!(condition.find("$_op") != StringRef::npos) ^
+                               emitVerificationRequiringOp);
----------------
Wouldn't `attrPred.getCondition()` 5 lines above fail if `attrPred.isNull()` ?


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:394
+
+    if (!emitVerificationRequiringOp && !allowMissingAttr) {
+      body << "  if (!" << varName << ") return " << emitErrorPrefix
----------------
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.


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:405
+
+    if (!hasConditionToEmit) {
+      body << "  }\n  \n";
----------------
Could you put this condition above the previous one? They are mutually exclusive and it will make it easier to match generated braces. It will also let you drop the `&& hasConditionToEmit` from that condition, which is currently asymmetrical and confusing.


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:406
+    if (!hasConditionToEmit) {
+      body << "  }\n  \n";
+      continue;
----------------
Nit: I belive the codestyle says to remove trailing whitespace so we shouldn't have whitespace-only lines


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:1622
 
+  genAttributeVerifier(op, "this->getAttr", "emitOpError(",
+                       /*emitVerificationRequiringOp=*/true, verifyCtx, body);
----------------
We should document somewhere that one cannot assume anything about the order in which auto-generated attribute verifier get applied


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:1990
+    auto numElements = sizeAttr.getType().cast<ShapedType>().getNumElements();
+    if (numElements != {1}) {{
+      return emitError(loc, "'{0}' attribute for specifying {2} segments "
----------------
Nit: elide trivial braces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80842