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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: mlir/docs/Rationale/SideEffectsAndSpeculation.md:174
+In the above example, we added the side effect [MemReadAt<0, false>] to the
+source, indicating load operation read parts of value from memref at stage 0.
----------------
Thanks for providing a good doc for the "effectOnFullRegion", what about stage? Seems like stages aren't really needed in any of these examples?


================
Comment at: mlir/include/mlir/Dialect/GPU/IR/GPUOps.td:1226
+                   Arg<AnyMemRef, "", [MemWriteAt<1>]>:$dst,
+                   Arg<AnyMemRef, "", [MemReadAt<0>]>:$src);
   let results = (outs Optional<GPU_AsyncToken>:$asyncToken);
----------------
I'm not sure how to interpret the stages here: do we guarantee that all of the read happen before the write? 
What about a loop that copy one element at a time?


================
Comment at: mlir/include/mlir/Interfaces/SideEffectInterfaces.td:38
+class MemoryEffect<string effectName, Resource resource, int stage,
+                   bit fullRegion>
+  : SideEffect<MemoryEffectsOpInterface, effectName, resource, stage,
----------------
Do we have enums in TableGen? (I don't remember).

I rather read "Full" and "Partial" than true/false if possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156087