separate AffineMapAccessInterface from AffineRead/WriteOpInterface

[adstraw]

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

[ftynse]

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

[jbruestle]

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]

Addressing code review feedback.

[ftynse]

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]

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.

[dcaballe]

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.

[adstraw]

In D96284#2554537, @dcaballe wrote:

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.

[dcaballe]

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]

Thanks for the feedback @dcaballe

[bondhugula]

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

[bondhugula]

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

[bondhugula]

Please also make the commit title more descriptive.

[adstraw]

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

[bondhugula]

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?

[adstraw]

Good catch @bondhugula.

[bondhugula]

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.

[bondhugula]

LGTM, thanks. Please do resolve the remaining comments.

[adstraw]

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.

mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td

InterfaceMethod<
       /*defaultImplementation=*/[{
         ConcreteOp op = cast<ConcreteOp>(this->getOperation());
         return op.getOperand(op.getStoredValOperandIndex());
       }]
     >,
   ];
 }
 
 def AffineMapAccessInterface : OpInterface<"AffineMapAccessInterface"> {
   let description = [{
-      Interface to query characteristics of MemRef Dereferencing ops.
+      Interface to query the AffineMap associated with a given MemRef.
+      The default implementation assumes an operation with a  single MemRef,
+      but the interface can extended to support multiple MemRefs as well.
   }];
 
   let methods = [
     InterfaceMethod<
       /*desc=*/"Returns the AffineMapAttr associated with 'memref'.",
       /*retTy=*/"NamedAttribute",
       /*methodName=*/"getAffineMapAttrForMemRef",
       /*args=*/(ins "Value":$memref),
       /*methodBody=*/[{}],
       /*defaultImplementation=*/[{
         ConcreteOp op = cast<ConcreteOp>(this->getOperation());
         assert(memref == op.getMemRef());
         return {Identifier::get(op.getMapAttrName(), op.getContext()),
                 op.getAffineMapAttr()};
       }]
     >,
   ];
 }
 
 #endif // AFFINEMEMORYOPINTERFACES