Page MenuHomePhabricator

[mlir][SideEffects] Define a set of interfaces and traits for defining side effects
ClosedPublic

Authored by rriddle on Feb 11 2020, 1:13 PM.

Details

Summary

This revision introduces the infrastructure for defining side-effects and attaching them to operations. This infrastructure allows for defining different types of side effects, that don't interact with each other, but use the same internal mechanisms. At the base of this is an interface that allows operations to specify the different effect instances that are exhibited by a specific operation instance. An effect instance is comprised of the following:

  • Effect: The specific effect being applied. For memory related effects this may be reading from memory, storing to memory, etc.
  • Value: A specific value, either operand/result/region argument, the effect pertains to.
  • Resource: This is a global entity that represents the domain within which the effect is being applied.

MLIR serves many different abstractions, which cover many different domains. Simple effects are may have very different context, for example writing to an in-memory buffer vs a database. This revision defines uses this infrastructure to define a set of initial MemoryEffects. The are effects that generally correspond to memory of some kind; Allocate, Free, Read, Write.

This set of memory effects will be used in follow revisions to generalize various parts of the compiler, and make others more powerful(e.g. DCE).

This infrastructure was originally proposed here:
https://groups.google.com/a/tensorflow.org/g/mlir/c/v2mNl4vFCUM

Depends On D74438

Diff Detail

Event Timeline

rriddle created this revision.Feb 11 2020, 1:13 PM

Just very quick skim, but could you add tests here already?

mlir/include/mlir/IR/SideEffects.td
2

Isn't the extra --- normally on the left? https://llvm.org/docs/CodingStandards.html#file-headers

rriddle updated this revision to Diff 244227.Feb 12 2020, 11:09 AM

Add tests

rriddle updated this revision to Diff 244229.Feb 12 2020, 11:12 AM

Update header comment

rriddle updated this revision to Diff 244230.Feb 12 2020, 11:14 AM

Resolve remaining comments.

rriddle marked 2 inline comments as done.Feb 12 2020, 11:18 AM

Just very quick skim, but could you add tests here already?

I had some spread throughout this revision stack, but I moved/added a bunch here.

mlir/include/mlir/IR/SideEffects.td
2

I copied this from one of the other files, I'll have to go back and see which file that was.

jpienaar accepted this revision.Feb 22 2020, 11:15 AM
jpienaar marked 2 inline comments as done.

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

mlir/include/mlir/IR/OpDefinition.h
1281

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?

1322

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.

1439–1442

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 ?

1445

Document why is HasNoSideEffect used here? (from the comment I can see why, but it is a bit implicit :-))

1480–1482

Reflow ? Seem text lines could be packed.

mlir/include/mlir/IR/SideEffects.td
15

Isn't this MLIR_IR_SIDEEFFECTS ?

31

s/ of the effects that are exhibited by this operation and places them in effects./ the operation's effects into effects/ ?

(same below)

70

ID is implicit with template instantiation, perhaps focus comment on the user interface.

142

But it isn't a class :) [in the TableGen sense of the word]

169

This is an important constraint that could be a bit subtle ... I'm assuming the docs explain rationale :)

mlir/test/lib/TestDialect/TestDialect.cpp
380

Using both effect and affect, show off :)

This revision is now accepted and ready to land.Feb 22 2020, 11:15 AM
mehdi_amini accepted this revision.Feb 23 2020, 11:30 PM

Awesome!

mlir/include/mlir/IR/OpDefinition.h
1281

Meta question maybe but: what is an "effect" that isn't a "side-effect"?

1322

Google3 isn't picking on the .clang-format in mlir/

mlir/include/mlir/IR/SideEffects.td
169

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)

rriddle updated this revision to Diff 246581.Feb 25 2020, 2:53 PM
rriddle marked 16 inline comments as done.

Rebase and resolve comments

mlir/include/mlir/IR/OpDefinition.h
1281

I have the exact same thoughts as @mehdi_amini. It also keeps consistency with everything else we have, e.g. HasNoSideEffect.

1439–1442

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).

mlir/include/mlir/IR/SideEffects.td
169

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).

This revision was automatically updated to reflect the committed changes.