[PATCH] D79829: [mlir][Affine] Introduce affine memory interfaces

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added inline comments.


================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:2564-2581
+/// Get memref operand.
+Value AffineVectorLoadOp::getMemRef() {
+  return getOperand(getMemRefOperandIndex());
+}
+
+MemRefType AffineVectorLoadOp::getMemRefType() {
+  return getMemRef().getType().cast<MemRefType>();
----------------
mehdi_amini wrote:
> bondhugula wrote:
> > dcaballe wrote:
> > > bondhugula wrote:
> > > > I missed why we need to introduce these here? Shouldn't they be shared via the op interface method?
> > > Are you suggesting using the interface also as a base class? I assumed that interfaces shouldn't have any implementation. That could work for some methods but not all of them have the same implementation (e.g., `getMapOperands`). I could move those with the same implementation.
> > You are right that some of the ops that use this interface will not have the same implementation. But for my information,  @ftynse, @rriddle, can Op Interfaces have implementations for things that are guaranteed to be shared among all ops using them? I remember seeing a default implementation being provided somewhere but I may be wrong.  
> > 
> > Anyway, can these be moved into AffineLoadOpBase, AffineStoreOpBase? (extra class declarations)
> I believe OpInterface method are type-erased virtual function, they can have a default implementation in which case an op may or may override the default implementation.
> But for my information, @ftynse, @rriddle, can Op Interfaces have implementations for things that are guaranteed to be shared among all ops using them? 

If an interface function declared in ODS has a body, and the ops don't redeclare it with `DeclareOpInterfaceMethods`, the default implementation is used in all ops. https://mlir.llvm.org/docs/OpDefinitions/#operation-interfaces has some examples.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79829