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

Mon Jul 31 21:03:12 PDT 2023

mehdi_amini added a comment.

Seems better with the int :)

We should still provide more doc, and an example in particular.

Also update the title and the description to reflect the current version, and add a link to the RFC in the description please.

Finally, something that makes all this still not totally complete for safely do the kind of things you described in the RFC is the notion of May/Must associated with the effect, as well as the "partial/full region" aspect of the effect.

================
Comment at: mlir/include/mlir/Interfaces/SideEffectInterfaceBase.td:172
+
+  /// The effect happen stage.
+  int stage = effectStage;
----------------
Nit: this isn't a sentence?

================
Comment at: mlir/include/mlir/Interfaces/SideEffectInterfaces.td:78
+def MemWrite : MemWrite<DefaultResource, 0>;
+class MemWriteAt<int stage> : MemWrite<DefaultResource, stage>;

----------------
Seems like the stage isn't optional here: that is, it is implicitly 0 which I can imagine being different from "undefined".

It may be OK, but it can also be surprising mixing implicit/explicit effects.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156087