adding a nan sanitizer similar to our int sanitizer #6009
Conversation
adamomainz
requested review from
antiagainst,
zhanglx13 and
ptillet
as code owners
python/triton/runtime/interpreter.py
Outdated
| @@ -195,7 +208,10 @@ def _convert_float(input, input_dtype, output_dtype, rounding_mode): | |||
| significand[subnormal_index] = (significand[subnormal_index] << bit_pos[subnormal_index]) & ( | |||
| (1 << input_dtype.fp_mantissa_width) - 1) | |||
| # Prevent overflow and underflow | |||
| exponent_output = np.maximum(0, np.minimum((exponent - bias_input + bias_output), (1 << output_exponent_width) - 1)) | |||
There was a problem hiding this comment.
Too many diffs. Maybe you should run pre-commit
There was a problem hiding this comment.
Apologies for the extra changes in here as well pre commit decided to do more linting than I like
That's not expected actually
There was a problem hiding this comment.
agree let me remove pre-commit and try again
There was a problem hiding this comment.
@Jokeren apologies for the delay! This is finally cleaned up
| """ | ||
| if not builder.options.sanitize_nan: | ||
| return | ||
| input_cond = and_(equal(lhs,lhs,builder), equal(rhs,rhs,builder), builder) |
There was a problem hiding this comment.
Neat trick to detect NaN values!
| return tl.tensor(builder.create_fsub(input.handle, other.handle), input.type) | ||
| # int - int | ||
| elif scalar_ty.is_int(): | ||
| if sanitize_nan: |
There was a problem hiding this comment.
Probably not needed for ints?
| input, other = binary_op_type_checking_impl(input, other, builder) | ||
| scalar_ty = input.type.scalar | ||
| if sanitize_nan: |
There was a problem hiding this comment.
Is there a reason why this check is in common path while in other functions it is after checking for int/float type?
There was a problem hiding this comment.
this is the only path where we dont have a ptr check so I thought it would be cleaner to just do the check here seeing as though in both cases below we are currently checking
There was a problem hiding this comment.
Would it make sense to put it everywhere under if float for consistency?
There was a problem hiding this comment.
agreed especially after your comment about not needing this for ints. Also considering moving this to some ops that have a higher chance of NaN ie exponentials, sqrt, pow etc. what do you think?
There was a problem hiding this comment.
Agree, the set of ops that may require this is most likely different than for overflow.
Hey All - We have had a few debugging issues with nan values popping up that are extremely difficult to find. Adding a nan sanitizer to make everyones life a bit easier.
New contributor declaration
[x ] I am not making a trivial change, such as fixing a typo in a comment.
[x ] I have written a PR description following these
rules.
[x ] I have run
pre-commit run --from-ref origin/main --to-ref HEAD.Select one of the following.
/testforlittests/unittestfor C++ tests/python/testfor end-to-end testsSelect one of the following.
littests.littests 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.)