-
Notifications
You must be signed in to change notification settings - Fork 3
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
TosaToLinalg: Fix unsigned tosa.clamp #155
TosaToLinalg: Fix unsigned tosa.clamp #155
Conversation
|
8b524da
to
00dc051
Compare
We should add a failing test with
Such a test will be useful to point out there is a hardcoded 63-bit limit in this pass, to be dealt with whenever larger integer support comes in. |
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.
Looks good!
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.
LGTM
One conceptual issue with clamp in general which we could think about fixing:
The tosa spec defines that the input+output type as well as the type of the attributes (min/max) need to be the same. It would be neat to enforce this (table gen + verifier), as it would prevent people from writing clip ranges which do not make sense for the given data type (as the case tested here, with unsigned inputs and a -1 lower bound).
Thanks! I add such test now. |
Lowering tosa.clamp with unsigned integer element types needs to use
cmpi ult
instead ofcmpi slt
,and the lower/upper bound must be interpreted as unsigned when converting to to arith.constants.