[PATCH] D74439: [mlir][SideEffects] Define a set of interfaces and traits for defining side effects

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini accepted this revision.
mehdi_amini added a comment.

Awesome!



================
Comment at: mlir/include/mlir/IR/OpDefinition.h:1192
+//===----------------------------------------------------------------------===//
+// Operation Side-Effect Modeling
+//===----------------------------------------------------------------------===//
----------------
jpienaar wrote:
> Nit: Effect modelling seems like the generic/more abstract part and side-effects more specific, which makes me think these should have the opposite nesting. WDYT?
Meta question maybe but: what is an "effect" that isn't a "side-effect"?


================
Comment at: mlir/include/mlir/IR/OpDefinition.h:1233
+  /// Returns a unique instance for the given effect class.
+  template <typename DerivedEffect> static DerivedEffect *get() {
+    static_assert(std::is_base_of_v<Effect, DerivedEffect>,
----------------
jpienaar wrote:
> Perhaps older version of clang-format/version that isn't picking up local config ... I'm assuming you'll run clang-format to fix this before.
Google3 isn't picking on the .clang-format in mlir/


================
Comment at: mlir/include/mlir/IR/SideEffects.td:168
+// not any visible mutation or dereference. This effect may only be placed on
+// a result or region argument.
+class MemAlloc<string resourceName>
----------------
jpienaar wrote:
> This is an important constraint that could be a bit subtle ... I'm assuming the docs explain rationale :)
I don't understand the reason for "this effect may only be placed on result or region argument", is this explained somewhere else? (maybe link from here if so)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74439