-
Notifications
You must be signed in to change notification settings - Fork 251
Add __builtin_spirv_ internal builtins #3374
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
Conversation
10864c2 to
4318704
Compare
Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]>
4318704 to
524c334
Compare
|
New LLVM intrinsic work is in progress, some draft implementation can be seen here: MrSidims/llvm-project@512a282 . But as I have said in https://discourse.llvm.org/t/rfc-spir-v-way-to-represent-float8-in-llvm-ir/87758 - we still need some builtins for the translator for older LLVM branches. |
Signed-off-by: Sidorov, Dmitry <[email protected]>
|
Applied most of the comments from @YuriPlyakhin . Thanks! |
Signed-off-by: Sidorov, Dmitry <[email protected]>
YuriPlyakhin
left a comment
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, there are just couple nit comments left.
maarquitos14
left a comment
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.
In general, LGTM, just a few details and nits here and there. A general comment, more of a nit actually, I think code is more readable when explicit types are used instead of auto (except if they are nasty, unreadable types such as iterators or similar). It's just a personal preference, so feel free to ignore.
| std::optional<ExtensionID> getRequiredExtension() const override { | ||
| if (isTypeFloat(16, FPEncodingBFloat16KHR)) | ||
| return ExtensionID::SPV_KHR_bfloat16; | ||
| if (isTypeFloat(8, FPEncodingFloat8E4M3EXT) || |
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.
Nit: We repeat this double check a few times, might be worth considering a utility function to group them (e.g. IsTypeFP8).
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.
There were some reasons, why I decided to drop such utility functions, but I don't remember particularly why. Lets push few other changes in the future and then decide if we want to add them.
| ; RUN: FileCheck < %t.spt %s --check-prefix=CHECK-SPIRV | ||
| ; RUN: llvm-spirv %t.spv -o %t.rev.bc -r --spirv-target-env=SPV-IR | ||
| ; RUN: llvm-dis %t.rev.bc -o %t.rev.ll | ||
| ; RUN: FileCheck < %t.rev.ll %s --check-prefix=CHECK-LLVM |
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.
Applies to all tests: should we add a spirv-val check?
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.
spirv-val conversions_scalar_vector.spv
error: line 7: Invalid capability operand: 4212
spirv-val is not yet ready
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.
Should we still add it with RUNx or add at least a TODO?
| @@ -0,0 +1,25 @@ | |||
| ; This test checks, that function with __builtin_spirv placed in the middle of | |||
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.
The test doesn't actually do this: it checks if a function with __spirv_builtin placed in the middle of the name is translated correctly. It's subtly different, but I think what we want to do is what is described here, so the problem is in the function name and the checks, right?
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.
Changed this 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.
Sorry, I wasn't clear. The internal builtin prefix is __builtin_spirv, but the function name we are trying to call is _Z18boo__spirv_builtinfs. My understanding is that we would want to test _Z18boo__builtin_spirv_fs instead. Is that right?
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.
Got you, updated the test
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.
Note that the order is still reversed: the internal builtin prefix is __builtin_spirv, but the function name contains __spirv_builtin. My understanding is that we want to check if having the internal builtin prefix in the middle of a function causes any trouble, and the function name does not contain the internal builtin prefix. Ping me offline if you need further clarification, I feel like I'm not being able to deliver my message correctly.
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.
Ah, indeed, thanks!
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
Way they are implemented is described in: #3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
Way they are implemented is described in: #3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
Way they are implemented is described in: #3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
Way they are implemented is described in: #3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
Way they are implemented is described in: #3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
Way they are implemented is described in: #3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
…#3374) Way they are implemented is described in: KhronosGroup#3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
Way they are implemented is described in: #3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
Way they are implemented is described in: #3221 The PR also adds SPV_EXT_float8 extension and packed conversions for SPV_INTEL_int4 Currently only conversion instructions (and internal builtins) are covered. TODO: in the following PR Saturation decoration will be added. Signed-off-by: Sidorov, Dmitry <[email protected]> --------- Signed-off-by: Sidorov, Dmitry <[email protected]>
Way they are implemented is described in:
#3221
The PR also adds SPV_EXT_float8 extension and packed conversions for
SPV_INTEL_int4
Currently only conversion instructions (and internal builtins) are
covered.
TODO: in the following PR Saturation decoration will be added.
Signed-off-by: Sidorov, Dmitry [email protected]