[PATCH] D74440: [mlir][SideEffects] Enable specifying side effects directly on the arguments/results of an operation.

[Jacques Pienaar via Phabricator via llvm-commits]

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

Should `Arg<"The MemRef to load from." ` be `Arg<"the MemRef to load from"` ? E.g., sentence fragment, not capitalized, could be added in generated docs as table entries?



================
Comment at: mlir/include/mlir/IR/OpBase.td:1538
+// A base decorator class that may optionally be added to OpVariables.
+class OpVariableDecorator {}
+
----------------
s/{}/;/ ?


================
Comment at: mlir/include/mlir/TableGen/SideEffects.h:26
+public:
+  // Return the name of the c++ effect.
+  StringRef getName() const;
----------------
Nit: C++


================
Comment at: mlir/lib/TableGen/Operator.cpp:283
     Record *argDef = dyn_cast<DefInit>(argumentValues->getArg(i))->getDef();
+    if (argDef->isSubClassOf(opVarClass))
+      argDef = argDef->getValueAsDef("constraint");
----------------
Mmm, why are we referring to "constraint" a few times, could this be stored instead or queried in a different way? (same as being done for operands and attributes)


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:1114
+    StringRef baseEffect = it.second.front().effect.getBaseName();
+    auto effectsParam = Twine("SmallVectorImpl<SideEffects::EffectInstance<" +
+                              baseEffect + ">> &effects")
----------------
Does formatv read easier here?


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:1132
+           << "::get()";
+      if (location.kind != EffectKind::Static)
+        body << ", value";
----------------
Comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74440