-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Determine indirectly defined constant for clamp op #2268
base: main
Are you sure you want to change the base?
Conversation
62fd183
to
ac19198
Compare
Constant can be converted/reshaped/broadcasted and then used in stablehlo.clamp op. Determine the base constant value for min/max and use them in ttir.clamp op.
ac19198
to
5ebaa5e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on ttnn docs and source code it seems that ttnn.clamp
op supports providing either scalars or tensors as min
and max
operands.
Here is the breakdown in third party dialects:
- StableHLO supports providing either scalars or tensors as
min
andmax
operands - ONNX and TOSA support only scalars as
min
andmax
operands - linalg doesn't seem to have a clamp/clip equivalent
- torch-mlir has separate ops:
aten.clamp
which supports scalar operands, andaten.clamp.Tensor
which supports tensor operands
Based on all of this, it seems like the most general way to support clamp/clip op in TTIR would be to do the same as StableHLO does and support providing either scalars or tensors as min
and max
operands, that way all third party dialects' equivalents will be able to lower to it without any additional logic. That means removing all this decomposition logic from the StableHLO->TTIR conversion, updating the TTIR and TTNN ops, and updating the runtime function for ttnn.clamp
op.
What do others think? @sdjordjevicTT @nsmithtt @AleksKnezevic
Yes this seems like a better approach, we probably didn't have ttnn clamp or something when we initially brought the op up. |
When I initially brought up StableHLO does not support scalars as min/max; instead it uses zero rank tensors in case of scalars. We can use the same approach like stablehlo as Marko suggested. However, we will still need decomposition (creation of zero rank tensors and broadcasting them) for scalar min/max as tt-metal does not support implicit broadcast of the operands for clamp op. We can determine whether to use scalar or tensor interface. I think, it will be better to always use tensor-based interface to keep Other possible solutions can be
Stablehlo based approach seems more suitable to me. |
I'll request to unblock this PR and merge it as it will unblock the models bring up with tt-torch. One of the model is failing due to this clamp op problem. I'll implement the generalize approach and we will have to sync with tt-forge front end as well; so that changes in both repos get merge simultaneously. |
Can you file / link an issue? Otherwise this sounds like a good approach to me, @mrakitaTT thoughts? |
Oh I see, I missed that, yeah we can't easily map the zero-rank tensor arguments to the As for the mapping from stablehlo to the one with constant value attributes, I believe that it will always be a situation like this (maybe some reshape/broadcast ops in between, or
which would get converted into:
for which we can create a fussion pass and convert it into this when the arguments tensors have a 0-rank:
Another tool we can use in situations like this is StableHLO interpreter, they listed const-folding as one of the use cases here.
Yeah, let's file the issue @mmanzoorTT and I will review this PR now to unblock, then we can come back to refactor this. |
ttmlir::utils::replaceOpWithNewDPSOp<ttir::MinimumOp>( | ||
rewriter, srcOp, outputType, maximumOp.getResult(0), adaptor.getMax()); | ||
rewriter, srcOp, outputType, maximumOp.getResult(0), max); | ||
|
||
return success(); | ||
} | ||
|
||
private: | ||
std::optional<float> getConstantValue(Value value) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you actually already have the logic here to extract the constant values to use. I would then avoid delaying the proper solution for later and just do it right now since it doesn't require much more work (to add two separate clamp ops to ttir and ttnn, and map here to the one with constant value attributes, and remove this decomposition logic). One model being blocked in tt-torch doesn't seem like that big of priority to me, please let me know if you disagree.
Ticket
closes #2185
Problem description
stablehlo.clamp is lowered to either 1) ttir.clamp or 2) ttir.maximum followed by ttir.minimum op.
However, tt-metal does not support implicit broadcast for maximum and minimum ops. These two
ops fail if the operand shapes does not match and broadcast is required.
What's changed
Checklist