Page MenuHomePhabricator

Add a new Dialect interface to control implicit attribute propagation.
Needs ReviewPublic

Authored by mehdi_amini on Jan 25 2021, 2:02 PM.

Details

Reviewers
rriddle
Summary

This interface allows for dialect attributes to be implicitly propagated through
rewrites, by augmenting the OpBuilder with a DictionnaryAttr member. It is
implicitly initialized to the current op attributes during pattern rewriting or
when the builder is created from an operation.

Diff Detail

Event Timeline

mehdi_amini created this revision.Jan 25 2021, 2:02 PM
mehdi_amini requested review of this revision.Jan 25 2021, 2:02 PM
bondhugula added inline comments.
mlir/include/mlir/IR/AttributePropagationInterface.h
23

Typo: implicitly

31

the attr -> `attr`

39

Typo: Dictionary

mlir/include/mlir/IR/Builders.h
202

Needs a comment.

481

Typo: spell fix.

mlir/lib/IR/AttributePropagationInterface.cpp
6

Missing file doc comment - here and for the header file.

18

Can hoist this while getting it from op:

MLIRContext *ctx = op->getContext()

20

Comment somewhere for this block.

mlir/lib/IR/Builders.cpp
400

Needs a comment.

mlir/test/Transforms/test-canonicalize.mlir
55

Nit: Drop extra new line.

bondhugula requested changes to this revision.Jan 25 2021, 6:46 PM
bondhugula added inline comments.
mlir/lib/IR/Builders.cpp
397

This doc comment will need an update. It's not just creating an operation from the fields in state.

This revision now requires changes to proceed.Jan 25 2021, 6:46 PM
bondhugula added inline comments.Jan 25 2021, 6:56 PM
mlir/include/mlir/IR/AttributePropagationInterface.h
32

-> propagate ... to op.

mlir/include/mlir/IR/Builders.h
199

This doc comment needs an update as well.

499

Rephrase for clarity. 'considered for propagation' based on what? Mention about the interface.

rriddle added inline comments.Jan 25 2021, 7:05 PM
mlir/include/mlir/IR/AttributePropagationInterface.h
11

Some of these are unnecessary.

42

nit: propagateDialectAttributes?

mlir/include/mlir/IR/Builders.h
202

Can you just call setPropagatedAttributes here?

mlir/lib/IR/AttributePropagationInterface.cpp
22

I'm afraid that this is going to be really really slow in-practice. Is this an indicator that it's about time we make "Dialect-Prefixed Attributes" more "first-class" in the API? Right now we use Identifier for attribute names but we could beef that up to be something more stateful. We already try to query state on these dialect attributes, and already feel the pain of the clunky string slicing+dialect lookup+interface lookup.

This seems to me like a 'tip of the iceberg' kind of solution. What happens if the attribute needs to be transformed? What happens if there is not a 1:1 correspondance between input operations and output operations in a transformation? etc. etc. I'm not sure what the right solution is, but this seems to be a narrowly scoped infrastructure solution to a tensorflow problem. I'd be much happier if this solved other obvious problems.

mlir/include/mlir/IR/Builders.h
485

"dictionary'

bondhugula added inline comments.Jan 25 2021, 7:56 PM
mlir/include/mlir/IR/Builders.h
202

All created ops would get the attributes of an op that was used for the insert point: This looks arbitrary outside of all pattern rewriter use cases! (where there is no replacement but say just IR injection happening) It looks like you'll need a new OpBuilder ctor or explicitly using setPropagatedAttributes on the builder when needed?

500

camelBack.

mlir/lib/IR/AttributePropagationInterface.cpp
17–28

This overhead on the creation of every op is a concern. Even if there are no dialect name prefixed attributes, you are cycling through all of the "insert point op's" attributes (op's intrinsic attributes as well as any other attributes) in all cases where the OpBuilder was created this way. It is also weird that there would be a workaround to avoid this overhead by creating the OpBuilder via its "insert point" ctor (propagated_attrs would be empty in that case) or some of the other ctors?

mlir/test/Transforms/test-canonicalize.mlir
59

Missing : after CHECK-NOT

62

Testing missing an attribute without any dialect prefix.

mehdi_amini added a comment.EditedJan 25 2021, 8:00 PM

This seems to me like a 'tip of the iceberg' kind of solution. What happens if the attribute needs to be transformed? What happens if there is not a 1:1 correspondance between input operations and output operations in a transformation?

There are two categories of attributes: dependent and dialect.

It seems to me that your comments are mostly applicable to "dependent attributes", i.e. attributes that are associated to an operation. This is different from the dialect attributes which intended to added as annotation on existing operations: these come with their own verifier for example, while dependent attributes are supposed to be verified by the operation verifier.

(I think it is best discussed in the RFC thread though, and leave implementation details comment to the revision here)

bondhugula added a comment.EditedJan 26 2021, 12:49 AM

This seems to me like a 'tip of the iceberg' kind of solution. What happens if the attribute needs to be transformed? What happens if there is not a 1:1 correspondance between input operations and output operations in a transformation? etc. etc. I'm not sure what the right solution is, but this seems to be a narrowly scoped infrastructure solution to a tensorflow problem. I'd be much happier if this solved other obvious problems.

I don't see this as a solution to a narrow or a tensorflow problem. In fact, this is a pretty generic solution to propagate attributes that are currently being lost -- and the situations this approach addresses is that of the attributes logically associated to a specific dialect. This would also encourage folks to prefix free attribute names with dialect name prefixes for preservation -- but that may not always be meaningful.

@mehdi_amini Anything further on this?

This revision now requires review to proceed.Jan 10 2022, 7:11 AM

Not just now: we've been looking into alternatives, it takes time to pan out because it could be much more intrusive. I need to get back to this...