Op traits are C++ classes. As such, traits can be derived from other traits. An example is
SingleBlockImplicitTerminator, which is derived from
Use Case: The inliner should not inline IR with unstructured control flow into ops that are annotated with
SingleBlockImplicitTerminator. Such IR would be invalid (Github issue #64978).
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
SingleBlockImplicitTerminator has a template parameter.)
Possible Solution: Implement both
SingleBlockImplicitTerminator explicitly. Do not use inheritance (https://reviews.llvm.org/D159078). The inliner can then check for
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.
Note: There are also
MaxSizedRegion<n>, etc., but those cannot be queried from C++ at all.
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>,
// 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:
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.
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?
SingleBlockImplicitTerminator::Impl no longer inherits from
TraitBase. Is this intended?
Do we actually need a specialized
SingleBlockImplicitTerminator ops? We could change
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