Repository navigation
Add support for fastmath 32-bit floating point divide - #6788
Conversation
gmarkall
left a comment
gmarkall
left a comment
There was a problem hiding this comment.
I think the general approach here looks good - I have a few comments about the implementation and testing on the diff. I think with my suggested changes to the maybe_fast_truediv function, the following should be true:
- A division by zero involving
float32operands in a kernel jitted withdebug=Truewill raise aZeroDivisionErrorfollowing the launch. - No exception should be raised following the launch of a kernel jitted with
fastmath=True, debug=True.
If this is the case, could you add a test that tests both these conditions hold please?
| self.assertIn('div.approx.ftz.f32', fastver.ptx) | ||
| self.assertNotIn('div.approx.ftz.f32', precver.ptx) |
There was a problem hiding this comment.
In addition to replacing the check here, we could also check that the non-fastmath version uses the non-ftz version of the divide too:
| self.assertIn('div.approx.ftz.f32', fastver.ptx) | |
| self.assertNotIn('div.approx.ftz.f32', precver.ptx) | |
| self.assertIn('div.approx.ftz.f32', fastver.ptx) | |
| self.assertNotIn('div.approx.ftz.f32', precver.ptx) | |
| self.assertIn('div.rn.f32', precver.ptx) | |
| self.assertNotIn('div.rn.f32', fastver.ptx) |
| with cgutils.if_zero(builder, args[1]): | ||
| context.error_model.fp_zero_division(builder, ("division by zero",)) |
There was a problem hiding this comment.
I think this should be moved inside the else branch - if fastmath is on, we don't want to be emitting code to raise on a division by zero.
| fast_replacement = 'fast_fdividef' | ||
| actual_libfunc = getattr(libdevice, fast_replacement) | ||
| sig = typing.signature(float32, float32, float32) | ||
| libfunc_impl = context.get_function(actual_libfunc, sig) | ||
| return libfunc_impl(builder, args) |
There was a problem hiding this comment.
Perhaps this can be simplified a bit, i.e.:
| fast_replacement = 'fast_fdividef' | |
| actual_libfunc = getattr(libdevice, fast_replacement) | |
| sig = typing.signature(float32, float32, float32) | |
| libfunc_impl = context.get_function(actual_libfunc, sig) | |
| return libfunc_impl(builder, args) | |
| sig = typing.signature(float32, float32, float32) | |
| impl = context.get_function(libdevice.fast_fdividef, sig) | |
| return impl(builder, args) |
(I think the pattern using getattr fitted for the trig functions etc. below, but isn't necessary for getting the implementation for a single function)
|
|
||
| class TestFastMathOption(CUDATestCase): | ||
| @skip_on_cudasim('fast divide not available in CUDASIM') | ||
| def test_kernel(self): |
There was a problem hiding this comment.
For a while I was wondering what this is checking that differs from what test_divf checks for below... Eventually I concluded that it tests that casting an integer to a float32 then using it in a division results in a div.approx instruction being emitted - given how finicky casts and the type system can be, I think it's good to keep this check alongside test_divf - Could you add a comment here explaining that this is testing the cast of an int being used in the divide, to make it a bit easier for future readers to see the difference?
Alternatively - do you have a different view on the difference between this and test_divf? I reached my conclusion by trying to derive meaning from the code (thinking backwards a bit) so maybe there's something else going on here that I haven't followed.
gmarkall
added
4 - Waiting on author
gmarkall
added
4 - Waiting on CI
gmarkall
left a comment
gmarkall
left a comment
There was a problem hiding this comment.
Many thanks for the changes - they look good to me. Tests pass locally with hardware and the simulator.
I think the doc failure is unrelated to the contents of this PR - it is either a transient HTTP failure or an issue that needs resolving with the Numba docs.
@esc Could this have a buildfarm run please?
|
Buildfarm ID: |
|
@esc How did it go? :-) |
Passed. |
stuartarchibald
left a comment
stuartarchibald
left a comment
There was a problem hiding this comment.
Thanks for the patch.
stuartarchibald
added
5 - Ready to merge
|
Not to resolve in this PR, but this raises an interesting question about flag compatibility, should fastmath and error model be tested to ensure compatibility? |
5 participants
This pull request adds support for generating more efficient code for 32-bit floating point divides in the presence of fastmath. This should be the final piece to: #6183.