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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

There is likely some documentation to update.

Also I'm not sure that "index" is expressive enough to carry the intent here. The closest I can relate here is pipeline stages in VLIW for example, so maybe the word "stage" should appear in the API?



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


================
Comment at: mlir/include/mlir/Interfaces/SideEffectInterfaceBase.td:173
+  /// The effect happen index.
+  string index = indexString;
 }
----------------
Why a string and not an int?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156087