This is an archive of the discontinued LLVM Phabricator instance.

separate AffineMapAccessInterface from AffineRead/WriteOpInterface
ClosedPublic

Authored by adstraw on Feb 8 2021, 11:13 AM.

Details

Summary

Separating the AffineMapAccessInterface from AffineRead/WriteOp interface so that dialects which extend Affine capabilities (e.g. PlaidML PXA = parallel extensions for Affine) can utilize relevant passes (e.g. MemRef normalization).

Diff Detail

Event Timeline

adstraw created this revision.Feb 8 2021, 11:13 AM
adstraw requested review of this revision.Feb 8 2021, 11:13 AM
adstraw updated this revision to Diff 322515.Feb 9 2021, 2:59 PM
ftynse requested changes to this revision.Feb 10 2021, 1:27 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
151

The name should refer to its connection with affine maps. std.load dereferences a memref just fine, but this interfaces wouldn't make sense for it. Something like AffineMapAccessInterface would be more suitable.

167–169

The interface arguments is only used for this assertion. Let's drop both the argument and the assertion...

mlir/lib/Transforms/Utils/Utils.cpp
86 ↗(On Diff #322515)

Use cast instead of dyn_cast if you don't check the result of cast for nullity.

Also, avoid using (dyn_)cast immediately after isa. Something like

auto derefInterface = dyn_cast<MemRefDereferencingInterface>(op);
if (!derefInterface) {
  // ...
  return ...;
}
NamedAttribute oldMapAttrPair = derefInterface.getAffineMapAttr();

should work here

This revision now requires changes to proceed.Feb 10 2021, 1:27 AM
jbruestle added inline comments.Feb 10 2021, 8:42 AM
mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
167–169

One of the primary reasons for making this interface is to allow support for memref normalization of other operations not defined in this repo. For our use case, this includes an operation which does affine dereferencing of multiple memrefs, so the argument is required to link the memref to the appropriate AffineMapAttr.

adstraw updated this revision to Diff 322708.Feb 10 2021, 9:11 AM
adstraw marked 2 inline comments as done.

Addressing code review feedback.

ftynse added inline comments.Feb 10 2021, 9:29 AM
mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
167–169

Then it needs to be appropriately documented. We have no knowledge of downstream projects and we can easily remove the code that is useless in core without warning unless it is clear from the code or documentation that it serves some purpose.

adstraw added inline comments.Feb 10 2021, 10:01 AM
mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
167–169

Note that AffineDmaStartOp uses this interface and provides a non-default implementation of the getAffineMapAttrForMemRef method which returns a different AffineMap depending on whether the src or dst MemRef is passed in. I will update the description to make this more clear, as well.

adstraw updated this revision to Diff 322726.Feb 10 2021, 10:02 AM
adstraw marked an inline comment as not done.
adstraw updated this revision to Diff 322729.Feb 10 2021, 10:06 AM

Maybe there's something off on my side but I can't see the diff between the last version of the patch (Diff 3) and Base. When I set up that view in "Revision Contents->History", it shows the Diff against a previous version of the patch, which doesn't include all the changes. Something seems to be off.

Maybe there's something off on my side but I can't see the diff between the last version of the patch (Diff 3) and Base. When I set up that view in "Revision Contents->History", it shows the Diff against a previous version of the patch, which doesn't include all the changes. Something seems to be off.

Apologize for the patching issues. I believe I have corrected the problem now. Diff 322729 shows all my intended changes from base.

It works after the last update. Thanks!
LGTM. Some minor comments. Please, wait for @ftynse's final approval.

mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
154–155
  • double space
  • can extended -> can be extended?
mlir/include/mlir/Dialect/Affine/IR/AffineOps.h
309 ↗(On Diff #322729)

Maybe adding a comment here about this being the implementation of a method of the new AffineMapAccessInterface interface would be useful.

adstraw updated this revision to Diff 322748.Feb 10 2021, 10:49 AM
adstraw marked 2 inline comments as done.

Thanks for the feedback @dcaballe

adstraw retitled this revision from add Affine MemRefDereferencing interface to add AffineMapAccessInterface.Feb 10 2021, 10:53 AM
bondhugula requested changes to this revision.Feb 10 2021, 11:11 AM

Mostly minor comments to address.

mlir/include/mlir/Dialect/Affine/IR/AffineOps.h
195 ↗(On Diff #322748)

Terminate all comments with a full stop please.

309 ↗(On Diff #322748)

Likewise.

mlir/lib/Transforms/Utils/Utils.cpp
77–81 ↗(On Diff #322748)

Nit: please use braces here (per LLVM coding style).

This revision now requires changes to proceed.Feb 10 2021, 11:11 AM

Thanks for making the "memref deferencing ops" thing more systematic here. This is looking good to me. Mostly polishing and doc related comments.

mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
153–154

I think we really need a couple more lines explaining the meaning of the interface better (especially for new users). For eg., there is an implication that the memref is being dereferenced / accessed - you actually have it in the name of the interface but this isn't reflected in the description here.

167–169

Assertion message please.

mlir/lib/Transforms/Utils/Utils.cpp
75–76 ↗(On Diff #322748)

Better name here: affMapAcc -> affMapAccInterface (no increase in number of lines due to this).

Please also make the commit title more descriptive.

adstraw updated this revision to Diff 322860.Feb 10 2021, 4:35 PM
adstraw marked 9 inline comments as done.
adstraw retitled this revision from add AffineMapAccessInterface to separate AffineMapAccessInterface from AffineRead/WriteOpInterface .Feb 10 2021, 4:39 PM
adstraw edited the summary of this revision. (Show Details)

Thanks for the feedback @ftynse and @bondhugula. Let me know if I missed anything.

bondhugula added inline comments.Feb 11 2021, 6:09 PM
mlir/lib/Transforms/Utils/Utils.cpp
31 ↗(On Diff #322860)

This isn't the only user of such a utility:

/lib/Dialect/Affine/Transforms/AffineLoopInvariantCodeMotion.cpp:64:static bool isMemRefDereferencingOp(Operation &op) {
./lib/Transforms/LoopFusion.cpp:72:static bool isMemRefDereferencingOp(Operation &op) {

Can we remove those as well?

bondhugula requested changes to this revision.Feb 11 2021, 6:09 PM
This revision now requires changes to proceed.Feb 11 2021, 6:09 PM
adstraw updated this revision to Diff 323345.Feb 12 2021, 8:54 AM
adstraw marked an inline comment as done.

Good catch @bondhugula.

bondhugula requested changes to this revision.Feb 13 2021, 12:46 AM
bondhugula added inline comments.
mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
153–154

I still find this documentation not direct/complete enough when read by a new user. It's missing information that clearly specifies which ops should be implementing this interface. Consider rephrasing along:

access a given memref -> dereference and access a given memref operand
154

assume -> assumes

154

default implementation -> current implementation?
Because you'd need to change the API when handling multiple memrefs.

155

to -> on

160–163

You won't need a set method in the future?

168

to match operation memref -> to match memref operand

mlir/include/mlir/Dialect/Affine/IR/AffineOps.td
758 ↗(On Diff #323345)

Nit: Update comment for the renamed argument.

mlir/lib/Transforms/Utils/Utils.cpp
263–265 ↗(On Diff #323345)

Drop trivial braces - unrelated change as well.

269–271 ↗(On Diff #323345)

Likewise - inadvertent change unrelated to this revision.

This revision now requires changes to proceed.Feb 13 2021, 12:46 AM
bondhugula accepted this revision.Feb 13 2021, 12:47 AM

LGTM, thanks. Please do resolve the remaining comments.

This revision now requires review to proceed.Feb 13 2021, 12:47 AM
ftynse resigned from this revision.Feb 15 2021, 1:10 AM
This revision is now accepted and ready to land.Feb 15 2021, 1:10 AM
adstraw marked 8 inline comments as done.Feb 16 2021, 10:37 AM
adstraw added inline comments.
mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
154

The reference to the "default implementation" in the description was done to address earlier comments regarding the need to pass in the memref to the interface at all + the need for the assert memref == op.getMemRef() in the default implementation. The reason for this is that operations implementing the interface can operate on more than one memref. I have the description to say that --- "Implementers of this interface must operate on at least one memref operand." I hope this clears things up.

160–163

I would prefer to address this in the future, as needed.

adstraw updated this revision to Diff 324049.Feb 16 2021, 10:39 AM
adstraw marked an inline comment as done.
flaub added a subscriber: flaub.Feb 16 2021, 12:30 PM