[PATCH] D156087: [MLIR] Add index to side effect

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

I still think documentation is missing, I don't see a definition of "Stage" yet?



================
Comment at: mlir/include/mlir/Interfaces/SideEffectInterfaces.h:190
+    auto sideEffectStage =
+        dicAttr.template getAs<IntegerAttr>("SideEffectStageAttr");
+    if (sideEffectStage) {
----------------
It goes with my comment below, but attributes are suboptimal here. If stages is important why not add an Optional<int> stage as member for this class?


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:3204
+      "    mlir::IntegerAttr stageAttr = "
+      "mlir::IntegerAttr::get(mlir::IndexType::get(getContext()), {0});\n"
+      "    SmallVector<mlir::NamedAttribute, 1> attrs;\n"
----------------
Using attributes does not seem optimal to me: it's far from free and we shouldn't put these creation here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156087