[PATCH] D79829: [mlir][Affine] Introduce affine memory interfaces
Alex Zinenko via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 19 05:22:24 PDT 2020
ftynse added a comment.
**TL;DR: "read" part is mostly correct. Use explicit `this->getOperation()` because class templates+inheritance. Also, cast<MemRefType>(....getType()) is incorrect, use ....getType().cast<MemRefType>() instead.**
In D79829#2037697 <https://reviews.llvm.org/D79829#2037697>, @dcaballe wrote:
> I tried to change all the interface methods to have a default implementation but I'm hitting some problems. After reading again the documentation about interfaces, it's not clear to me what is the difference between providing a `methodBody` or a `defaultImplementation`. I had to dig into the generated code to have a bit better understanding but still, I couldn't make it work. I uploaded the patch with both approaches: methods in `AffineWriteOpInterface` has a `methodBody`, methods in `AffineReadOpInterface` has a `defaultImplementation`.
Let's make a detour to try and untangle this (maybe we can update the doc afterwards). This is probably the most C++-intense part of the code base. I believe OpInterfaces implementation was inspired by the concept-based polymorphism from the inheritance is the base class of evil <https://sean-parent.stlab.cc/papers-and-presentations/#value-semantics-and-concept-based-polymorphism> talk, transposed to LLVM and MLIR infrastructure (in particular LLVM-style casts <https://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html> and MLIR traits <https://mlir.llvm.org/docs/Traits/#defining-a-trait>). The code example <https://sean-parent.stlab.cc/presentations/2013-09-06-inheritance/value-semantics-unique.cpp> from the talk implements the concept-based polymorphic object that stores a pointer to the underlying object and dispatches polymorphic function calls to free functions differentiated by the type (stored as template parameter) of their first argument.
// This is the "interface" with pure virtual methods.
struct Concept {
virtual void interfaceMethod() = 0;
};
template <typename Derived>
struct Model {
void interfaceMethod() override {
interfaceMethodImpl(data);
}
Derived data;
};
// This is a concrete implementation that does not need inheritance.
struct Concrete;
// And this is the implementation of interfaceMethod for Concrete.
void interfaceMethodImpl(Concrete) {}
// This class has polymorphic behavior without having virtual functions itself.
// We can see it as actual user-visible interface for our purposes, users
// will get instances of this class and will be able to work with them opaquely.
struct Polymorphic {
template <typename Actual>
Polymorphic(Actual &&a) : container(new Model<Actual>{forward<Actual>(a)}) {}
void interfaceMethod() { container->interfaceMethod(); }
std::unique_ptr<Concept> container;
};
Now, in MLIR, we don't actually need to store the underlying object because, as far as interfaces are concerned, there underlying object is an `Operation` that belongs to a block->region->parent-op->...->top-level-module-op that the caller owns. It would suffice to store a bare non-owning `Operation *`. Furthermore, we would like interface to integrate with the LLVM-style casting mechanisms. We're in luck, `template <ConcereteOp, ...> class Op<ConcreteOp, ...>` does exactly that: store an `Operation *` and provide `isa/dyn_cast` support. Now we start shifting a bit from the original concept-based idea with more inheritance and into the following:
struct Concept {
// We will always work on some operation
virtual void interfaceMethod(Operation *) = 0;
};
template <typename Derived>
struct Model : public Concept {
void interfaceMethod(Operation *op) override {
// Call the implementation from the Derived class, which is expected
// to have a method with the compatible signature. (We could also
// stick with free functions).
cast<Derived>(op).interfaceMethod();
}
// No need to store the data anymore.
};
struct Polymorphic : public Op<Polymorphic, ...> {
Poymorphic(Operation *op) : Op<Polymorphic, ...>(op), impl(...) {}
void interfaceMethod() {
impl->interfaceMethod(getOperation());
}
Concept *impl;
};
The remaining questions are how to we construct an instance of `Model<Derived>` that we could store in `impl` and how could we partially share the implementation. That's where the Traits mechanism comes into play. MLIR implements the Traits pattern that allows one to add generalized functions into specific Op classes. The scheme is roughly the following
template <typename Concrete>
struct Trait {
void traitMethod() {
// This has access to actual operation and its type through CRTP.
// cast<Concrete>(getOperation());
};
Operation *getOperation() {
// In practice, this is bit more complex because it's provided by the base class
// TraitBase through another layer of CRTP instead of requiring every Trait to
// reimplement this from scratch.
return static_cast<Concrete *>(this)->getOperation();
}
};
// Again, the actual implementation is a more involved because of several layers
// of CRTP and a class variadic template Traits<...> that we use.
struct ConcreteOp : public Trait<ConcreteOp> {
// This will have "traitMethod" available by inheritance from Trait.
};
Trait mechanism is also used in different places like verifiers and various precondition checkers, but for our purposes here it's basically a collection of templated base class _members_. The key difference with interfaces is that traits are allowed to have (static) data. We can then use this capability to store our instance of `Model<Derived>` in the trait. As an additional bonus, we get to reuse the `Traits<...>` machinery already implemented in operations. Now, since we are already giving all Ops that implement the interface a trait to support the interface, we might also use the properties of the trait itself -- in particular provide generic implementations of the methods that will be automatically available in all ops that have the trait (or the interface from which the trait is derived). The final scheme resembles the following
struct Concept {
virtual void interfaceMethod(Operation *) = 0;
};
template <typename Derived>
struct Model : public Concept {
void interfaceMethod(Operation *op) override {
cast<Derived>(op).interfaceMethod();
};
};
template <typename Derived>
struct Trait {
static Concept &instance() {
// This is where the per-class instance of Model lives.
static Model<Derived> singleton;
return singleton;
}
// We can define (or not!) the interface method implementation that
// will be available in concrete ops by inheritance.
void interfaceMethod() {
}
};
struct Interface : public Op<Interface, ...> {
Interface(Operation *op) : Op<Interface, ...(op) {
// We leverage Op registration to store enable looking up Trait::instance in
// an opaque way.
impl = op->getAbstractOperation()->getInterfaceFor<Interface>();
}
void interfaceMethod() {
impl->interfaceMethod();
}
Concept *impl;
};
struct ConcereteOp : public Op<ConcreteOp, Trait, ...> {
};
As usual, the actual implementation is more involved for practical purposes (e.g., Concept and Model are both nested inside the `detail::InterfaceTraits` class [not to be confused with Trait], Trait containing the `instance` is nested inside `OpInterface` to share the implementation and each specific `Interface` has a nested `Trait` that inherits `OpInterface::Trait` [both are class templates btw] with additional implementations, all `Trait`s inherit from CRTP-base TraitBase, etc). Some of this complexity is hidden by table-genning Concept, Model, Interface and nested Trait classes. Unfortunately, sometimes it bites you back.
Two sources of confusion here are (1) the presence of identically-named functions that may or may not be inheritance-related and (2) the relation between Interfaces and Traits. For (1), concrete Ops are ultimately expected to implement exactly the signature described in Tablegen. These functions will be called by casting `Operation *` to the specific subclass of `Op`. Functions with identical signature will be provided in the `Interface` class, the rest is the dispatching mechanism letting them call similarly-named functions in the specific subclass of `Op` without directly knowing about them. For (2), Interfaces are extension of `Op` and are therefore wrappers around `Operation *` which can be stored or passed around; Traits are a way to share type-parameterized implementations across different `Op` subclasses. Interfaces rely on Traits to operate. Traits can be used to share the implementation of functions Interfaces require from Ops.
Finally, I can answer the original question about the difference between "methodBody" and "defaultImplementation". "methodBody" is the body of `Model::interfaceMethod`, if not provided in tablegen, it will just dispatch to `ConcreteOp::interfaceMethod`. Since there is no inheritance between `Model<ConcreteOp>` and `ConcreteOp`, these methods are not automatically available in `ConcreteOp`; on the contrary, they must be implemented for the entire mechanism to work. "defaultImplementation" is the body of "Trait::interfaceMethod" and will be automatically available in `ConcreteOp`. Combined with empty "methodBody", it will be the implementation that the interface will call into by default, hence the name. It can, however, be overridden by `ConcreteOp` and the interface will then call into that because it casts to `ConcreteOp`.
After this long detour, answers to your questions are relatively straightforward.
> The problem that I'm hitting with `methodBody` is that the **interface methods are not visible from the concrete op**.
Interface methods are not visible because there is no inheritance from interface. Trait methods (referred to as default implementations) _are_ visible.
> For example, I cannot call `getMemRef()` using an `AffineStoreOp` object. Looking at the generated code, I see that the method `getMemRef()` is autogenerated for the class `AffineWriteOpInterface`, which is implemented like this:
>
> Value AffineReadOpInterface::getMemRef() {
> return getImpl()->getMemRef(getOperation());
> }
>
>
> However, there is no `getMemRef()` declared/defined in the same way for `AffineStoreOp` or `AffineVectorStoreOp`. Shouldn't they have similar autogenerated code? Am I missing something?
An interface is like OOP interface in e.g. Java, it expects all classes that implement it to define the interface methods themselves. We have the possibility of providing a default implementation for them.
> For `defaultImplementation`, I followed the doc example so interface methods are defined like this:
>
> InterfaceMethod<
> /*desc=*/[{ Returns the memref operand to read from. }],
> /*retTy=*/"Value",
> /*methodName=*/"getMemRef",
> /*args=*/(ins),
> /*methodBody*/[{}],
> /*defaultImplementation=*/ [{
> ConcreteOp op = cast<ConcreteOp>(getOperation());
> return op.getOperand(op.getMemRefOperandIndex());
> }]
> >,
>
>
>
> However, the compiler complains about invoking `getOperation()` without object:
>
> error: cannot call member function ‘mlir::Operation* mlir::Op<ConcreteT
> ype, Traits>::getOperation() [with ConcreteType = mlir::AffineReadOpInterface; Traits = {}]’ without object
Because default implementation is placed in the Trait class template, which inherits from another class template (`template <typename ConcreteOp> struct AffineReadOpInterfaceTrait : public OpInterface<AffineReadOpInterface, detail::AffineReadOpInterfaceInterfaceTraits>::Trait<ConcreteOp>`) we need to explicitly disambiguate the call to the parent method (`getOperaiton` is defined in OpState, inherited by Op, inherited by OpInterface). `this->getOperation()` will work smoothly.
> I get a bit lost in the internal details but `ConcreteType = mlir::AffineReadOpInterface` looks suspicious. Shouldn't it be `AffineLoadOp`/`AffineVectorLoadOp`?
No, it's correct. ConcreteType is a template parameter that points to the derived interface class for downcasting. If it were `AffineLoadOp`, we could erroneously assume any Op that implements `AffineReadOpInterface` _isa_ `AffineLoadOp`.
> I would appreciate some help, @ftynse @mehdi_amini @rriddle.
The following diff makes everything compile and pass tests.
diff --git a/mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td b/mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
index d9e8789a07a..8738000d8d5 100644
--- a/mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
+++ b/mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
@@ -29,7 +29,7 @@ def AffineReadOpInterface : OpInterface<"AffineReadOpInterface"> {
/*args=*/(ins),
/*methodBody*/[{}],
/*defaultImplementation=*/ [{
- ConcreteOp op = cast<ConcreteOp>(getOperation());
+ ConcreteOp op = cast<ConcreteOp>(this->getOperation());
return op.getOperand(op.getMemRefOperandIndex());
}]
>,
@@ -40,8 +40,8 @@ def AffineReadOpInterface : OpInterface<"AffineReadOpInterface"> {
/*args=*/(ins),
/*methodBody=*/[{}],
/*defaultImplementation=*/[{
- ConcreteOp op = cast<ConcreteOp>(getOperation());
- return llvm::cast<MemRefType>(op.getMemRef().getType());
+ ConcreteOp op = cast<ConcreteOp>(this->getOperation());
+ return op.getMemRef().getType().template cast<MemRefType>();
}]
>,
InterfaceMethod<
@@ -51,7 +51,7 @@ def AffineReadOpInterface : OpInterface<"AffineReadOpInterface"> {
/*args=*/(ins),
/*methodBody=*/[{}],
/*defaultImplementation=*/[{
- ConcreteOp op = cast<ConcreteOp>(getOperation());
+ ConcreteOp op = cast<ConcreteOp>(this->getOperation());
return llvm::drop_begin(op.getOperands(), 1);
}]
>,
@@ -63,7 +63,7 @@ def AffineReadOpInterface : OpInterface<"AffineReadOpInterface"> {
/*args=*/(ins),
/*methodBody=*/[{}],
/*defaultImplementation=*/[{
- ConcreteOp op = cast<ConcreteOp>(getOperation());
+ ConcreteOp op = cast<ConcreteOp>(this->getOperation());
return op.getAffineMapAttr().getValue();
}]
>,
@@ -82,21 +82,33 @@ def AffineWriteOpInterface : OpInterface<"AffineWriteOpInterface"> {
/*retTy=*/"Value",
/*methodName=*/"getMemRef",
/*args=*/(ins),
- /*methodBody=*/ [{ return op.getOperand(op.getMemRefOperandIndex()); }]
+ /*methodBody=*/[{}],
+ /*defaultImplementation=*/[{
+ ConcreteOp op = cast<ConcreteOp>(this->getOperation());
+ return op.getOperand(op.getMemRefOperandIndex());
+ }]
>,
InterfaceMethod<
/*desc=*/[{ Returns the type of the memref operand. }],
/*retTy=*/"MemRefType",
/*methodName=*/"getMemRefType",
/*args=*/(ins),
- /*methodBody=*/[{ return llvm::cast<MemRefType>(getMemRef().getType()); }]
+ /*methodBody=*/[{}],
+ /*defaultImplementation=*/[{
+ ConcreteOp op = cast<ConcreteOp>(this->getOperation());
+ return op.getMemRef().getType().template cast<MemRefType>();
+ }]
>,
InterfaceMethod<
/*desc=*/[{ Returns affine map operands. }],
/*retTy=*/"Operation::operand_range",
/*methodName=*/"getMapOperands",
/*args=*/(ins),
- /*methodBody=*/[{ return llvm::drop_begin(op.getOperands(), 2); }]
+ /*methodBody=*/[{}],
+ /*defaultImplementation=*/[{
+ ConcreteOp op = cast<ConcreteOp>(this->getOperation());
+ return llvm::drop_begin(op.getOperands(), 2);
+ }]
>,
InterfaceMethod<
/*desc=*/[{ Returns the affine map used to index the memref for this
@@ -104,7 +116,11 @@ def AffineWriteOpInterface : OpInterface<"AffineWriteOpInterface"> {
/*retTy=*/"AffineMap",
/*methodName=*/"getAffineMap",
/*args=*/(ins),
- /*methodName=*/[{ return op.getAffineMapAttr().getValue(); }]
+ /*methodName=*/[{}],
+ /*defaultImplementation=*/[{
+ ConcreteOp op = cast<ConcreteOp>(this->getOperation());
+ return op.getAffineMapAttr().getValue();
+ }]
>,
];
}
> Thanks,
> Diego
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79829/new/
https://reviews.llvm.org/D79829
More information about the llvm-commits
mailing list