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

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar accepted this revision.
jpienaar marked 2 inline comments as done.
jpienaar added a comment.
This revision is now accepted and ready to land.

Nice, looking forward to using (whatever we end up calling) "dynamic" op side effects :)



================
Comment at: mlir/include/mlir/IR/OpDefinition.h:1192
+//===----------------------------------------------------------------------===//
+// Operation Side-Effect Modeling
+//===----------------------------------------------------------------------===//
----------------
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?


================
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>,
----------------
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.


================
Comment at: mlir/include/mlir/IR/OpDefinition.h:1350-1353
+/// This trait indicates that the side effects of an operation includes the
+/// effects of operations nested within its regions. If the operation has no
+/// derived effects interfaces, the operation itself can be assumed to have no
+/// side effects.
----------------
A little bit fuzzy for me.

So this is, an op that may have side-effects depending on the whether the operands in its regions have side-effects. Or that is probably too specialized, e.g., I'm excluding attributes, esp. function attributes, but one could also have an attribute "has_side_effect" that determines if the op has side-effects.

Recursive makes me think of descending into regions, but I don't know if it captures attributes that affect effects. Perhaps this was already discussed in the discourse thread ... but would HasDynamicSideEffect ? HasOptionalSideEffect ? MaybeHasSideEffect ?


================
Comment at: mlir/include/mlir/IR/OpDefinition.h:1356
+class HasRecursiveSideEffects
+    : public TraitBase<ConcreteType, HasNoSideEffect> {};
+} // namespace OpTrait
----------------
Document why is HasNoSideEffect used here? (from the comment I can see why, but it is a bit implicit :-))


================
Comment at: mlir/include/mlir/IR/OpDefinition.h:1391-1393
+/// The following effect indicates that the operation writes to some
+/// resource. A 'write' effect implies only mutating a resource, and
+/// not any visible dereference or read.
----------------
Reflow ? Seem text lines could be packed.


================
Comment at: mlir/include/mlir/IR/SideEffects.td:14
+
+#ifndef MLIR_SIDEEFFECTS
+#define MLIR_SIDEEFFECTS
----------------
Isn't this MLIR_IR_SIDEEFFECTS ?


================
Comment at: mlir/include/mlir/IR/SideEffects.td:30
+    InterfaceMethod<[{
+        Collects all of the effects that are exhibited by this operation and
+        places them in `effects`.
----------------
s/ of the effects that are exhibited by this operation and places them in `effects`./ the operation's effects into `effects`/ ?

(same below)


================
Comment at: mlir/include/mlir/IR/SideEffects.td:69
+    /// Collect all of the effect instances that correspond to the given effect
+    /// ID and place them in 'effects'.
+    template <typename Effect> void getEffects(
----------------
ID is implicit with template instantiation, perhaps focus comment on the user interface.


================
Comment at: mlir/include/mlir/IR/SideEffects.td:141
+
+// This class represents the definition for the memory effects interface. Users
+// should generally not use this directly, and should instead use
----------------
But it isn't a class :) [in the TableGen sense of the word]


================
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>
----------------
This is an important constraint that could be a bit subtle ... I'm assuming the docs explain rationale :)


================
Comment at: mlir/test/lib/TestDialect/TestDialect.cpp:366
+
+    // Check for a result to affect.
+    Value value;
----------------
Using both effect and affect, show off :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74439