Repository navigation
Add support for fp16 comparison native operators #8010
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
Changes from 1 commit
c33108a
c353f64
0752ccd
d9abed6
5fe0392
9ead75c
4a2006d
77b745e
5663396
35fd82f
0b6d045
6e02ce8
9911814
da6016a
400ac81
8be1d0c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ | |
| signature, Registry) | ||
| from numba.cuda.types import dim3, grid_group | ||
| from numba import cuda | ||
|
|
||
| import operator | ||
|
|
||
| registry = Registry() | ||
| register = registry.register | ||
|
|
@@ -366,6 +366,20 @@ class Cuda_fp16_cmp(ConcreteTemplate): | |
| return Cuda_fp16_cmp | ||
|
|
||
|
|
||
| def _genfp16_comparison_operator(l_key): | ||
| @register_global(l_key) | ||
| class Cuda_fp16_cmp2(AbstractTemplate): | ||
| key = l_key | ||
|
|
||
| def generic(self, args, kws): | ||
| assert not kws | ||
| if len(args) == 2 and \ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain the thinking behind the logic here? Some questions that spring to mind when I read this implementation:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Further to this, I notice that the following implementation passes all tests: def generic(self, args, kws):
assert not kws
if args == (types.float16, types.float16):
return signature(types.b1, types.float16, types.float16)Does this imply that there is some missing test coverage, or that the simple implementation suffices? (my suspicion is that the simple implementation suffices, but I don't think I've understood yet why the more complex implementation could be needed, so I'm not certain either way).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gmarkall requiring both arguments to be float16 will fail, forcing one argument to be float16 ended up generating incorrect mixed type code. As an example comparing a float16 and float32 converted the float32 to float16 and then doing the comparison which is incorrect. The types int16 and int8 can be converted to float16.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thankyou for the information - however, I don't really understand the explanation. I have the following patch applied on top of your commit d9abed6: diff --git a/numba/cuda/cudadecl.py b/numba/cuda/cudadecl.py
index c0ef42652..a54740efd 100644
--- a/numba/cuda/cudadecl.py
+++ b/numba/cuda/cudadecl.py
@@ -5,7 +5,6 @@ from numba.core.typing.templates import (AttributeTemplate, ConcreteTemplate,
AbstractTemplate, CallableTemplate,
signature, Registry)
from numba.cuda.types import dim3, grid_group
-from numba.core.typeconv import Conversion
from numba import cuda
import operator
@@ -374,17 +373,8 @@ def _genfp16_comparison_operator(l_key):
def generic(self, args, kws):
assert not kws
-
- if len(args) == 2 and \
- (args[0] == types.float16 or args[1] == types.float16):
- if (args[0] == types.float16):
- convertible = self.context.can_convert(args[1], args[0])
- else:
- convertible = self.context.can_convert(args[0], args[1])
-
- if (convertible == Conversion.exact) or \
- (convertible == Conversion.promote):
- return signature(types.b1, types.float16, types.float16)
+ if args == (types.float16, types.float16):
+ return signature(types.b1, types.float16, types.float16)
Cuda_hadd = _genfp16_binary(cuda.fp16.hadd)I run the testsuite like this: and I get the following summary as a result: From this I can see three possibilities:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gmarkall I added two test cases that verify the generated ptx. One of these test cases fails when using your code that requires both arguments to be float16.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks - this now makes sense. I now recall that the complexity here is to work around the fact that we can't have multiple concrete templates for a single global. I think what's going on here is going to be quite unclear to anyone coming across this for the first time - can you please add some comments explaining the background and how the workaround works to the |
||
| (args[0] == types.float16 or args[1] == types.float16): | ||
| return signature(types.b1, types.float16, types.float16) | ||
|
|
||
| return Cuda_fp16_cmp2 | ||
|
gmarkall marked this conversation as resolved.
Resolved
|
||
|
|
||
|
|
||
| Cuda_hadd = _genfp16_binary(cuda.fp16.hadd) | ||
| Cuda_hsub = _genfp16_binary(cuda.fp16.hsub) | ||
| Cuda_hmul = _genfp16_binary(cuda.fp16.hmul) | ||
|
|
@@ -374,11 +388,17 @@ class Cuda_fp16_cmp(ConcreteTemplate): | |
| Cuda_hneg = _genfp16_unary(cuda.fp16.hneg) | ||
| Cuda_habs = _genfp16_unary(cuda.fp16.habs) | ||
| Cuda_heq = _genfp16_binary_comparison(cuda.fp16.heq) | ||
| Cuda_eq = _genfp16_comparison_operator(operator.eq) | ||
|
gmarkall marked this conversation as resolved.
Resolved
|
||
| Cuda_hne = _genfp16_binary_comparison(cuda.fp16.hne) | ||
| Cuda_ne = _genfp16_comparison_operator(operator.ne) | ||
| Cuda_hge = _genfp16_binary_comparison(cuda.fp16.hge) | ||
| Cuda_ge = _genfp16_comparison_operator(operator.ge) | ||
| Cuda_hgt = _genfp16_binary_comparison(cuda.fp16.hgt) | ||
| Cuda_gt = _genfp16_comparison_operator(operator.gt) | ||
| Cuda_hle = _genfp16_binary_comparison(cuda.fp16.hle) | ||
| Cuda_le = _genfp16_comparison_operator(operator.le) | ||
| Cuda_hlt = _genfp16_binary_comparison(cuda.fp16.hlt) | ||
| Cuda_lt = _genfp16_comparison_operator(operator.lt) | ||
|
|
||
|
|
||
| # generate atomic operations | ||
|
|
||
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.
Why the
2?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.
Because it was determined in this PR: #7885
with input from @sklam that operator registration requires using 'AbstractTemplate' while intrinsics need to use 'ConcreteTemplate'.
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.
To clarify my thinking - I think the name
Cuda_fp16_cmp2is a little confusing - there is the other class in the preceding method, but there's no name collision so it wouldn't have been necessary to call it...cmp2as opposed to..cmp- the difference between this class and the one from the preceding method is that this one is for typing comparisons using the operator module, so naming itCuda_fp16_operator_cmp(for example) would be more descriptive.