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

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/IR/OpDefinition.h:1192
+//===----------------------------------------------------------------------===//
+// Operation Side-Effect Modeling
+//===----------------------------------------------------------------------===//
----------------
mehdi_amini wrote:
> 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"?
I have the exact same thoughts as @mehdi_amini. It also keeps consistency with everything else we have, e.g. HasNoSideEffect.


================
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.
----------------
jpienaar wrote:
> 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 ?
This is explicitly for the region case. This is effectively stating that the effects of operations within nested regions should be included when considering the effects of this operation. For example, if we have the following:

```mlir
affine.for ... {
  affine.store ...
} 
```

The affine.for itself is side effect free, but for analysis sake we also consider the effects of its body. So an analysis could look at affine.for and say "this loop has a write". This also allows for treating lambda like operations as side-effect free:

```
%foo = my.lambda { 
                 std.store ...
             } : () -> i32 
```

%foo has no side effects, as it does not execute the body.

The optional side effect case is already handled by the interface itself. The operation can freely query attributes/etc. before populating the set of effects that are applied. This was intentional so that we can support things like function/call attributes that define various effect bits(LLVM style).


================
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>
----------------
mehdi_amini wrote:
> 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)
There is no good reason other than forcing a specific usage pattern. I removed that comment. I can see use cases where a we allocate to an existing `Value`(e.g. placement new-esque).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74439