-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[Frontend] [BC breaking] Always follow C semantics on % #4955
base: main
Are you sure you want to change the base?
Conversation
The semantics of `%` in triton used to be type dependant (!!). With this PR, we make `%` always follow C semantics, similar to `//`. We update the type promotion docs fixing some inaccuracies. It is still not entirely precise though. For a discussion of the current semantics see triton-lang#4697
@@ -390,6 +390,7 @@ def _mod_operation_ill_conditioned(dtype_x, dtype_y) -> bool: | |||
('int64', 'float16'), | |||
('int64', 'float32'), | |||
('int64', 'float64'), | |||
('uint16', 'float32'), |
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.
it errored by a small percentage, so just bump the tolerance a bit
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.
ahh, that makes sense. I was wondering why the tests on the different archs became divergent. Thank you!
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.
From further testing locally on a 3090, it seems that the difference between triton and the ref arrays are much larger. The single mismatched element that is making the test fail has the following values:
triton = -5.173683166503906e-05
and numpy = 1.3480271
Almost seems like a type conversion problem.
If I add the ill-conditioned pair (uint16, float32) then it fails on the h100 seems rather odd since it fails when the ill-conditioned check isn't present.
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.
Yes, this test is completely cursed. It involves an incredible amount of UB that happens to work by chance in most cases, but sometimes it breaks, like here lol
Would you be keen on taking on the side-quest of improving the test, so that we don't sample numbers that are too large if we are going to cast them to a dtype where they don't fit? In particular, you'd get rid of _mod_operation_ill_conditioned
, and you'd just set a max
value for the sampling depending on both of the dtypes. I think this would fix this issue + would get rid of the horrible workaround of _mod_operation_ill_conditioned
.
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.
Ha, that is incredible, I can definitely take on the side-quest! I appreciate the direction, shouldn't be terribly difficult.
Continuation of the work from @lezcano #4698
Pretty sure all that was left were changes for the frem function to emit
np.fmod
instead ofnp.remainder
and to ignore ('uint16', 'float64') mod computations in the tests. I believe this combination is ill-conditioned but I could be wrong about that.The core Triton is a small number of people, and we receive many PRs (thank
you!). To help us review your code more quickly, if you are a new
contributor (less than 3 PRs merged) we ask that you complete the following
tasks and include the filled-out checklist in your PR description.
Complete the following tasks before sending your PR, and replace
[ ]
with[x]
to indicate you have done them.I am not making a trivial change, such as fixing a typo in a comment.
I have written a PR description following these
rules.
I have run
pre-commit run --from-ref origin/main --to-ref HEAD
.Select one of the following.
/test
forlit
tests/unittest
for C++ tests/python/test
for end-to-end testsit fixes tests based on the semantic changes for the % operator
.Select one of the following.
lit
tests.lit
tests I have added follow these best practices,including the "tests should be minimal" section. (Usually running Python code
and using the instructions it generates is not minimal.)