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

[donald chen via Phabricator via llvm-commits]

cxy-1993 added a comment.

Thank you for your guidance. I have update the patch according to your comment. @mehdi_amini



================
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.
----------------
mehdi_amini wrote:
> Thanks for providing a good doc for the "effectOnFullRegion", what about stage? Seems like stages aren't really needed in any of these examples?
I rewrote the first example and used a virtual SIMD operation to illustrate the effect of stages.







================
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);
----------------
mehdi_amini wrote:
> 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?
NICE CATCH! Stage modeling indeed not be suitable for operations that can lower to loops, as its primary application lies in SIMD-like operations where hardware guarantees the order of reads and writes. I have updated existing dialect's side effects and the descriptions in the documentation. 


================
Comment at: mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td:284
 
-  let results = (outs Res<MemRefRankOf<[AnyType], [1]>, "", [MemAlloc<DefaultResource>]>);
 
----------------
mehdi_amini wrote:
> We're losing the "DefaultResource" here, how does the "*At" compose with various resources?
DONE


================
Comment at: mlir/include/mlir/Interfaces/SideEffectInterfaceBase.td:172
+
+  /// The effect happen stage.
+  int stage = effectStage;
----------------
mehdi_amini wrote:
> Nit: this isn't a sentence?
I refactored this comment for better clarity.


================
Comment at: mlir/include/mlir/Interfaces/SideEffectInterfaceBase.td:173
+  /// The effect happen index.
+  string index = indexString;
 }
----------------
mehdi_amini wrote:
> Why a string and not an int?
I have changed it to int.


================
Comment at: mlir/include/mlir/Interfaces/SideEffectInterfaces.h:190
+    auto sideEffectStage =
+        dicAttr.template getAs<IntegerAttr>("SideEffectStageAttr");
+    if (sideEffectStage) {
----------------
mehdi_amini wrote:
> 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?
DONE


================
Comment at: mlir/include/mlir/Interfaces/SideEffectInterfaces.h:183
 
+  /// Return the effect happen index.
+  unsigned getIndex() const {
----------------
mehdi_amini wrote:
> Where is the notion of "index" defined? 
Changed to "stage" now.


================
Comment at: mlir/include/mlir/Interfaces/SideEffectInterfaces.td:38
+class MemoryEffect<string effectName, Resource resource, int stage,
+                   bit fullRegion>
+  : SideEffect<MemoryEffectsOpInterface, effectName, resource, stage,
----------------
mehdi_amini wrote:
> Do we have enums in TableGen? (I don't remember).
> 
> I rather read "Full" and "Partial" than true/false if possible.
I can not found any enum in TableGen except EnumAttr...


================
Comment at: mlir/include/mlir/Interfaces/SideEffectInterfaces.td:78
+def MemWrite : MemWrite<DefaultResource, 0>;
+class MemWriteAt<int stage> : MemWrite<DefaultResource, stage>;
 
----------------
mehdi_amini wrote:
> 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.
I have replace optional<int> member variable with default value null with int member variable with default value 0.


================
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"
----------------
mehdi_amini wrote:
> Using attributes does not seem optimal to me: it's far from free and we shouldn't put these creation here.
DONE


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156087