Additional verifications of TTIR dialect ops #1399
Conversation
azecevicTT
requested review from
sdjordjevicTT,
nsmithtt,
rpavlovicTT,
svuckovicTT,
mtopalovicTT,
mrakitaTT,
nobradovictt and
jserbedzijaTT
as code owners
azecevicTT
force-pushed
the
azecevic/ttir-tightening
branch
from
d1246c8 to
9c9d1d3
Compare
- Refactoring of ElementwiseOpInteface to better reflect intention, with fix of broadcast shape calculation, considering that operand that represetnts destination shouldn't affect output shape. - Check number of operands for AttrSizedOperandSegments ops with simple traits. - Minor refactoring of TTIR_GenericOp. Addresses #1289, but I would leave it as open to track further progress with similar traits and interfaces needed in the TTNN dialect.
azecevicTT
force-pushed
the
azecevic/ttir-tightening
branch
from
9c9d1d3 to
c169963
Compare
| @@ -172,8 +172,12 @@ def TTIR_DeallocOp : TTIR_Op<"dealloc"> { | |||
| // TTIR top level named ops | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| def TwoOperands : ParamNativeOpTrait<"NOperands", "2">; | |||
There was a problem hiding this comment.
@azecevicTT Can you leave comment in next PR to clarify that this is needed because eltwise operations accept variadic number of arguments in which case mlir doesn't enforce number of operands required for operation?
| return mlir::cast<mlir::RankedTensorType>(val.getType()).getShape(); | ||
| }; | ||
|
|
||
| const auto operandSegmentSizes = |
There was a problem hiding this comment.
@azecevicTT I wanted to ask also why the use of auto in whole function?
I understand using it when you can deduce type from rhs (i.e constructor call), but this is overuse imo. Do you have some reason for it?
There was a problem hiding this comment.
I can overuse auto sometimes and I am aware that it can hinder readability sometimes (language servers tends to mitigate this biggest disadvantage nowadays). The biggest advantage (besides the laziness, can't lie about that) is the form of polymorphism that auto gives us. In this example I don't care if the type of operandSegmentSizes is mlir::DenseI32Array, llvm::ArrayRef, llvm::SmallVector, std::vector, std::array, as long as it has indexing operation and size() member function (with reasonable semantics) it's good to go, and I don't have to change anything if the underlying type changes, because I'm committed only to the interface, not the type.
There was a problem hiding this comment.
I see your point about not worrying about the underlying type, but do you realistically expect the API to change? I’d also argue that this type of polymorphism can lead to bugs in certain scenarios. For example, imagine you initially use auto with a SmallVector and rely on its indexing operator and size() functionality. Later, the API changes and returns a DenseMap instead, where the indexing operator behaves differently from SmallVector.
I understand the value of auto for lambdas, iterators, and constructor expressions, but beyond that, it feels a bit excessive to me.
There was a problem hiding this comment.
I don't disagree that it doesn't have disadvantages (basically it's the static version of the duck typing), I just think that benefits that auto brings outweights those disadvantages. The main point is that interfaces are usually more stable than types, your producer is much more likely to change type than interface. Now, in these concrete example I don't expect it to change since the RHS already mentions concrete type in the template, but you can argue that's already a reason in itself to not mention it again in the declaration.
I'm not religious about using auto, I just think it was a nice addition to C++ for a lot of reasons, we should just make a project guidelines when it's ok to use it and when it's not.
There was a problem hiding this comment.
I wasn’t focusing on this specific type, but the fact that the entire function was using auto is what caught my attention...
This addresses #1289, but I would leave it open to track further progress on similar traits and interfaces needed in the TTNN dialect.