Op traits are C++ classes. As such, traits can be derived from other traits. An example is SingleBlockImplicitTerminator, which is derived from SingleBlock.
Use Case: The inliner should not inline IR with unstructured control flow into ops that are annotated with SingleBlock or SingleBlockImplicitTerminator. Such IR would be invalid (Github issue #64978).
Problem: Operation::hasTrait does not work for derived traits. I.e., op->hasTrait<OpTrait::SingleBlock> will return false even if SingleBlockImplicitTerminator is implemented. (At the same time, the SingleBlockImplicitTerminator cannot be checked for with hasTrait because SingleBlockImplicitTerminator has a template parameter.)
Possible Solution: Implement both SingleBlock and SingleBlockImplicitTerminator explicitly. Do not use inheritance (https://reviews.llvm.org/D159078). The inliner can then check for hasTrait<OpTrait::SingleBlock>().
I looked into improving the hasTrait implementation. This would work for Op<ConcreteType, class... Traits>::hasTrait <class Trait>(): We could check if Trait is among the base classes of Traits.... But it won’t work for getHasTraitFn, which is used by Operation::hasTrait. That function looks for a given Trait TypeID among the list of implemented traits (given as variadic template parameters). As far as I know, it is not possible to extend this check to look for base classes. (There is no C++ type_traits helper to get the base class of a class.
Alternatively, we could require that base traits are declared via a public using Base = <BASE TRAIT>;. I think then the above-mentioned check could be extended.
Any thoughts?
Note: There are also SizedRegion<n>, MaxSizedRegion<n>, etc., but those cannot be queried from C++ at all.
1 Like
+1
I think it’d make more sense to express the inheritance/implication in ODS rather than C++.
We already have interfaces capable of implying other interfaces. This could be ported to traits.
Right now, we also have TraitList which allows a list of traits to be implemented at once.
One can use this to implement it as well:
class SingleBlockImplicitTerminatorImpl<string op>
: ParamNativeOpTrait<"SingleBlockImplicitTerminator", op>,
StructuralOpTrait;
// Op's regions have a single block with the specified terminator.
class SingleBlockImplicitTerminator<string op>
: TraitList<[SingleBlockImplicitTerminatorImpl<op>, SingleBlock]>;
That’s a good idea, I didn’t know about TraitList. I updated https://reviews.llvm.org/D159078 as described.
This introduces an interesting topic:
SingleBlock implements push_back and insert member functions. However, SingleBlockImplicitTerminator does too, as the implementation of these members differs from SingleBlock’s. This way, using any of these functions in an operation with the SingleBlockImplicitTerminator will lead to a compilation error.
As SingleBlockImplicitTerminator implies SingleBlock and SingleBlockImplicitTerminator needs to shadow parts of SingleBlock’s interface, we may need a different mechanism to express this “inheritance”. Is C++ inheritance indeed the answer here?
Note: @matthias-springer SingleBlockImplicitTerminator::Impl no longer inherits from TraitBase. Is this intended?
Do we actually need a specialized push_back/insert for SingleBlockImplicitTerminator ops? We could change push_back/insert of SingleBlock such that it inserts before the terminator if the block has one.
No, this was a mistake.
This makes sense. I think we can go down that path, also avoiding potential errors when using SingleBlock::push_back/insert. I think it’d make sense I work on this, as our project (SYCL-MLIR) uses this interface and check-mlir will probably miss this.
I implemented this change here