[PATCH] D80420: [mlir] Expand operand adapter to take attributes

[Lei Zhang via Phabricator via llvm-commits]

antiagainst accepted this revision.
antiagainst added a comment.
This revision is now accepted and ready to land.

Nice, thanks Jacques!



================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:73
 // {0}: The name of the attribute specifying the segment sizes.
-const char *attrSizedSegmentValueRangeCalcCode = R"(
+const char *adapterAttrSizedSegmentValueRangeCalcCode = R"(
+  assert(odsAttrs && "missing attributes for op with segment sizes");
----------------
This is not really code for calculation. What about naming it as "adapterSegmentSizeAttrInitCode" or something?


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:74
+const char *adapterAttrSizedSegmentValueRangeCalcCode = R"(
+  assert(odsAttrs && "missing attributes for op with segment sizes");
+  auto sizeAttr = odsAttrs.get("{0}").cast<DenseIntElementsAttr>();
----------------
Nit: "missing segment size attribute for op" ?


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:77
+)";
+const char *opAttrSizedSegmentValueRangeCalcCode = R"(
   auto sizeAttr = getAttrOfType<DenseIntElementsAttr>("{0}");
----------------
Similarly here. What about "opSegmentSizeAttrInitCode"?


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:1862
+
+  auto sizeAttrInit = formatv(adapterAttrSizedSegmentValueRangeCalcCode,
+                              "operand_segment_sizes")
----------------
Nit: we can do `std::string sizeAttrInit = formatv()` to avoid the `.str()` at the end and be more explicit about the type.


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:1875
+         << "\n  return odsAttrs.get(\"" << name << "\").";
+    if (attr.isOptional() || attr.hasDefaultValue())
+      body << "dyn_cast_or_null<";
----------------
Do we want to construct the default value here like https://github.com/llvm/llvm-project/blob/master/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp#L368-L378?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80420