Skip to content

[HLSL] Finish exposing half types and intrinsics always #132804

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

Merged
merged 2 commits into from
Mar 24, 2025

Conversation

spall
Copy link
Contributor

@spall spall commented Mar 24, 2025

We previously made an implementation error when adding half overloads for HLSL library functionality. The half type is always defined in HLSL and half intrinsics should not be conditionally included.
When native 16-bit types are disabled half is a unique 32-bit float type with lesser promotion rank than float.

Apply pattern #81782 to intrinsics added in #95999.
Closes #132793

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics labels Mar 24, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 24, 2025

@llvm/pr-subscribers-backend-x86

Author: Sarah Spall (spall)

Changes

Finish the work of #81782
Closes #132793


Full diff: https://github.com/llvm/llvm-project/pull/132804.diff

1 Files Affected:

  • (modified) clang/lib/Headers/hlsl/hlsl_alias_intrinsics.h (+32-16)
diff --git a/clang/lib/Headers/hlsl/hlsl_alias_intrinsics.h b/clang/lib/Headers/hlsl/hlsl_alias_intrinsics.h
index 585e905c7bf5d..d1e23007f92ff 100644
--- a/clang/lib/Headers/hlsl/hlsl_alias_intrinsics.h
+++ b/clang/lib/Headers/hlsl/hlsl_alias_intrinsics.h
@@ -134,16 +134,18 @@ double4 abs(double4);
 /// \brief Returns the arccosine of the input value, \a Val.
 /// \param Val The input value.
 
-#ifdef __HLSL_ENABLE_16_BIT
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_acos)
 half acos(half);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_acos)
 half2 acos(half2);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_acos)
 half3 acos(half3);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_acos)
 half4 acos(half4);
-#endif
 
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_acos)
 float acos(float);
@@ -447,16 +449,18 @@ double4 asdouble(uint4, uint4);
 /// \brief Returns the arcsine of the input value, \a Val.
 /// \param Val The input value.
 
-#ifdef __HLSL_ENABLE_16_BIT
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_asin)
 half asin(half);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_asin)
 half2 asin(half2);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_asin)
 half3 asin(half3);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_asin)
 half4 asin(half4);
-#endif
 
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_asin)
 float asin(float);
@@ -475,16 +479,18 @@ float4 asin(float4);
 /// \brief Returns the arctangent of the input value, \a Val.
 /// \param Val The input value.
 
-#ifdef __HLSL_ENABLE_16_BIT
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan)
 half atan(half);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan)
 half2 atan(half2);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan)
 half3 atan(half3);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan)
 half4 atan(half4);
-#endif
 
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan)
 float atan(float);
@@ -505,16 +511,18 @@ float4 atan(float4);
 /// \param y The y-coordinate.
 /// \param x The x-coordinate.
 
-#ifdef __HLSL_ENABLE_16_BIT
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan2)
 half atan2(half y, half x);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan2)
 half2 atan2(half2 y, half2 x);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan2)
 half3 atan2(half3 y, half3 x);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan2)
 half4 atan2(half4 y, half4 x);
-#endif
 
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan2)
 float atan2(float y, float x);
@@ -721,16 +729,18 @@ float4 cos(float4);
 /// \brief Returns the hyperbolic cosine of the input value, \a Val.
 /// \param Val The input value.
 
-#ifdef __HLSL_ENABLE_16_BIT
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_cosh)
 half cosh(half);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_cosh)
 half2 cosh(half2);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_cosh)
 half3 cosh(half3);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_cosh)
 half4 cosh(half4);
-#endif
 
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_cosh)
 float cosh(float);
@@ -2124,16 +2134,18 @@ float4 sin(float4);
 /// \brief Returns the hyperbolic sine of the input value, \a Val.
 /// \param Val The input value.
 
-#ifdef __HLSL_ENABLE_16_BIT
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_sinh)
 half sinh(half);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_sinh)
 half2 sinh(half2);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_sinh)
 half3 sinh(half3);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_sinh)
 half4 sinh(half4);
-#endif
 
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_sinh)
 float sinh(float);
@@ -2215,16 +2227,18 @@ float4 step(float4, float4);
 /// \brief Returns the tangent of the input value, \a Val.
 /// \param Val The input value.
 
-#ifdef __HLSL_ENABLE_16_BIT
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tan)
 half tan(half);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tan)
 half2 tan(half2);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tan)
 half3 tan(half3);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tan)
 half4 tan(half4);
-#endif
 
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tan)
 float tan(float);
@@ -2243,16 +2257,18 @@ float4 tan(float4);
 /// \brief Returns the hyperbolic tangent of the input value, \a Val.
 /// \param Val The input value.
 
-#ifdef __HLSL_ENABLE_16_BIT
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tanh)
 half tanh(half);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tanh)
 half2 tanh(half2);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tanh)
 half3 tanh(half3);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tanh)
 half4 tanh(half4);
-#endif
 
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tanh)
 float tanh(float);

@llvmbot
Copy link
Member

llvmbot commented Mar 24, 2025

@llvm/pr-subscribers-clang

Author: Sarah Spall (spall)

Changes

Finish the work of #81782
Closes #132793


Full diff: https://github.com/llvm/llvm-project/pull/132804.diff

1 Files Affected:

  • (modified) clang/lib/Headers/hlsl/hlsl_alias_intrinsics.h (+32-16)
diff --git a/clang/lib/Headers/hlsl/hlsl_alias_intrinsics.h b/clang/lib/Headers/hlsl/hlsl_alias_intrinsics.h
index 585e905c7bf5d..d1e23007f92ff 100644
--- a/clang/lib/Headers/hlsl/hlsl_alias_intrinsics.h
+++ b/clang/lib/Headers/hlsl/hlsl_alias_intrinsics.h
@@ -134,16 +134,18 @@ double4 abs(double4);
 /// \brief Returns the arccosine of the input value, \a Val.
 /// \param Val The input value.
 
-#ifdef __HLSL_ENABLE_16_BIT
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_acos)
 half acos(half);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_acos)
 half2 acos(half2);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_acos)
 half3 acos(half3);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_acos)
 half4 acos(half4);
-#endif
 
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_acos)
 float acos(float);
@@ -447,16 +449,18 @@ double4 asdouble(uint4, uint4);
 /// \brief Returns the arcsine of the input value, \a Val.
 /// \param Val The input value.
 
-#ifdef __HLSL_ENABLE_16_BIT
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_asin)
 half asin(half);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_asin)
 half2 asin(half2);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_asin)
 half3 asin(half3);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_asin)
 half4 asin(half4);
-#endif
 
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_asin)
 float asin(float);
@@ -475,16 +479,18 @@ float4 asin(float4);
 /// \brief Returns the arctangent of the input value, \a Val.
 /// \param Val The input value.
 
-#ifdef __HLSL_ENABLE_16_BIT
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan)
 half atan(half);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan)
 half2 atan(half2);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan)
 half3 atan(half3);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan)
 half4 atan(half4);
-#endif
 
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan)
 float atan(float);
@@ -505,16 +511,18 @@ float4 atan(float4);
 /// \param y The y-coordinate.
 /// \param x The x-coordinate.
 
-#ifdef __HLSL_ENABLE_16_BIT
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan2)
 half atan2(half y, half x);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan2)
 half2 atan2(half2 y, half2 x);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan2)
 half3 atan2(half3 y, half3 x);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan2)
 half4 atan2(half4 y, half4 x);
-#endif
 
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_atan2)
 float atan2(float y, float x);
@@ -721,16 +729,18 @@ float4 cos(float4);
 /// \brief Returns the hyperbolic cosine of the input value, \a Val.
 /// \param Val The input value.
 
-#ifdef __HLSL_ENABLE_16_BIT
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_cosh)
 half cosh(half);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_cosh)
 half2 cosh(half2);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_cosh)
 half3 cosh(half3);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_cosh)
 half4 cosh(half4);
-#endif
 
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_cosh)
 float cosh(float);
@@ -2124,16 +2134,18 @@ float4 sin(float4);
 /// \brief Returns the hyperbolic sine of the input value, \a Val.
 /// \param Val The input value.
 
-#ifdef __HLSL_ENABLE_16_BIT
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_sinh)
 half sinh(half);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_sinh)
 half2 sinh(half2);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_sinh)
 half3 sinh(half3);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_sinh)
 half4 sinh(half4);
-#endif
 
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_sinh)
 float sinh(float);
@@ -2215,16 +2227,18 @@ float4 step(float4, float4);
 /// \brief Returns the tangent of the input value, \a Val.
 /// \param Val The input value.
 
-#ifdef __HLSL_ENABLE_16_BIT
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tan)
 half tan(half);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tan)
 half2 tan(half2);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tan)
 half3 tan(half3);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tan)
 half4 tan(half4);
-#endif
 
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tan)
 float tan(float);
@@ -2243,16 +2257,18 @@ float4 tan(float4);
 /// \brief Returns the hyperbolic tangent of the input value, \a Val.
 /// \param Val The input value.
 
-#ifdef __HLSL_ENABLE_16_BIT
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tanh)
 half tanh(half);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tanh)
 half2 tanh(half2);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tanh)
 half3 tanh(half3);
+_HLSL_AVAILABILITY(shadermodel, 6.2)
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tanh)
 half4 tanh(half4);
-#endif
 
 _HLSL_BUILTIN_ALIAS(__builtin_elementwise_tanh)
 float tanh(float);

Copy link
Contributor

@bob80905 bob80905 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, for abs, it still depends on the _HLSL_16BIT_AVAILABILITY availability attribute. Does this PR intend to keep abs overloads using half "unexposed"? Or should that overload for abs be exposed too?

@spall
Copy link
Contributor Author

spall commented Mar 24, 2025

For example, for abs, it still depends on the _HLSL_16BIT_AVAILABILITY availability attribute. Does this PR intend to keep abs overloads using half "unexposed"? Or should that overload for abs be exposed too?

I'm not sure I understand the question, I think abs is the same as the ones in this PR are modified to be.

@spall
Copy link
Contributor Author

spall commented Mar 24, 2025

For example, for abs, it still depends on the _HLSL_16BIT_AVAILABILITY availability attribute. Does this PR intend to keep abs overloads using half "unexposed"? Or should that overload for abs be exposed too?

Oh I see what you are referring to. That might be my mistake; let me double check if I used the wrong __HLSL_AVAILABILITY

@farzonl
Copy link
Member

farzonl commented Mar 24, 2025

Are there any test changes we should consider here? If not could you prefix [NFC] to the title?

@bob80905
Copy link
Contributor

For example, for abs, it still depends on the _HLSL_16BIT_AVAILABILITY availability attribute. Does this PR intend to keep abs overloads using half "unexposed"? Or should that overload for abs be exposed too?

Oh I see what you are referring to. That might be my mistake; let me double check if I used the wrong __HLSL_AVAILABILITY

Yeah, if the intention of this PR is to expose all half type overloads always, then I would think there is no more utility in defining
HLSL_16BIT_AVAILABILITY anymore.

@spall
Copy link
Contributor Author

spall commented Mar 24, 2025

For example, for abs, it still depends on the _HLSL_16BIT_AVAILABILITY availability attribute. Does this PR intend to keep abs overloads using half "unexposed"? Or should that overload for abs be exposed too?

Oh I see what you are referring to. That might be my mistake; let me double check if I used the wrong __HLSL_AVAILABILITY

Yeah, if the intention of this PR is to expose all half type overloads always, then I would think there is no more utility in defining HLSL_16BIT_AVAILABILITY anymore.

I think I was meant to use _HLSL_16BIT_AVAILABILITY and not _HLSL_AVAILABILITY; I'll fix that. Thanks for noticing the error!

Copy link
Collaborator

@llvm-beanz llvm-beanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not NFC, so we should verify that we can call these intrinsics with half values even if 16-bit types aren't enabled, and that they properly codegen to 32-bit variants.

@spall
Copy link
Contributor Author

spall commented Mar 24, 2025

Are there any test changes we should consider here? If not could you prefix [NFC] to the title?

It looked to me like the tests were all correct based on what was done in the reference PR

@llvm-beanz
Copy link
Collaborator

Finish the work of #81782

Maybe instead:

Apply pattern of #81782 to intrinsics added in #95999.

Also probably worth having a description of the problem this solves in the description so that it ends up in the final commit message.

@bob80905
Copy link
Contributor

For example, for abs, it still depends on the _HLSL_16BIT_AVAILABILITY availability attribute. Does this PR intend to keep abs overloads using half "unexposed"? Or should that overload for abs be exposed too?

Oh I see what you are referring to. That might be my mistake; let me double check if I used the wrong __HLSL_AVAILABILITY

Yeah, if the intention of this PR is to expose all half type overloads always, then I would think there is no more utility in defining HLSL_16BIT_AVAILABILITY anymore.

I think I was meant to use _HLSL_16BIT_AVAILABILITY and not _HLSL_AVAILABILITY; I'll fix that. Thanks for noticing the error!

What I'm trying to get at is, you're removing #ifdef __HLSL_ENABLE_16_BIT, and replacing it with an availability attribute making sure that the shader model is at least 6.2 for these functions.
If you look at the definition for HLSL_16bit_availability, you'll notice that it's also defined under an ifdef: #ifdef __HLSL_ENABLE_16_BIT
The essence of this PR seems to me like it's assuming from now on that #ifdef __HLSL_ENABLE_16_BIT will always be true.
So, that means this availability attribute will always be defined. And it's identical to the HLSL_AVAILABILITY attribute.
I might understand this incorrectly, but now it seems to me like there's no point or distinguishing use between HLSL_AVAILABILITY and HLSL_16bit_AVAILABILITY. Does that make sense?
Which would imply that functions like abs should just use HLSL_Availability instead and we can do away with HLSL_16bit_AVAILABILITY. Since you want all function overloads with a half parameter to always be exposed.

@spall
Copy link
Contributor Author

spall commented Mar 24, 2025

this is not NFC, so we should verify that we can call these intrinsics with half values even if 16-bit types aren't enabled, and that they properly codegen to 32-bit varia

For example, for abs, it still depends on the _HLSL_16BIT_AVAILABILITY availability attribute. Does this PR intend to keep abs overloads using half "unexposed"? Or should that overload for abs be exposed too?

Oh I see what you are referring to. That might be my mistake; let me double check if I used the wrong __HLSL_AVAILABILITY

Yeah, if the intention of this PR is to expose all half type overloads always, then I would think there is no more utility in defining HLSL_16BIT_AVAILABILITY anymore.

I think I was meant to use _HLSL_16BIT_AVAILABILITY and not _HLSL_AVAILABILITY; I'll fix that. Thanks for noticing the error!

What I'm trying to get at is, you're removing #ifdef __HLSL_ENABLE_16_BIT, and replacing it with an availability attribute making sure that the shader model is at least 6.2 for these functions. If you look at the definition for HLSL_16bit_availability, you'll notice that it's also defined under an ifdef: #ifdef __HLSL_ENABLE_16_BIT The essence of this PR seems to me like it's assuming from now on that #ifdef __HLSL_ENABLE_16_BIT will always be true. So, that means this availability attribute will always be defined. And it's identical to the HLSL_AVAILABILITY attribute. I might understand this incorrectly, but now it seems to me like there's no point or distinguishing use between HLSL_AVAILABILITY and HLSL_16bit_AVAILABILITY. Does that make sense? Which would imply that functions like abs should just use HLSL_Availability instead and we can do away with HLSL_16bit_AVAILABILITY. Since you want all function overloads with a half parameter to always be exposed.

@llvm-beanz Thoughts on this?

@spall
Copy link
Contributor Author

spall commented Mar 24, 2025

this is not NFC, so we should verify that we can call these intrinsics with half values even if 16-bit types aren't enabled, and that they properly codegen to 32-bit varia

For example, for abs, it still depends on the _HLSL_16BIT_AVAILABILITY availability attribute. Does this PR intend to keep abs overloads using half "unexposed"? Or should that overload for abs be exposed too?

Oh I see what you are referring to. That might be my mistake; let me double check if I used the wrong __HLSL_AVAILABILITY

Yeah, if the intention of this PR is to expose all half type overloads always, then I would think there is no more utility in defining HLSL_16BIT_AVAILABILITY anymore.

I think I was meant to use _HLSL_16BIT_AVAILABILITY and not _HLSL_AVAILABILITY; I'll fix that. Thanks for noticing the error!

What I'm trying to get at is, you're removing #ifdef __HLSL_ENABLE_16_BIT, and replacing it with an availability attribute making sure that the shader model is at least 6.2 for these functions. If you look at the definition for HLSL_16bit_availability, you'll notice that it's also defined under an ifdef: #ifdef __HLSL_ENABLE_16_BIT The essence of this PR seems to me like it's assuming from now on that #ifdef __HLSL_ENABLE_16_BIT will always be true. So, that means this availability attribute will always be defined. And it's identical to the HLSL_AVAILABILITY attribute. I might understand this incorrectly, but now it seems to me like there's no point or distinguishing use between HLSL_AVAILABILITY and HLSL_16bit_AVAILABILITY. Does that make sense? Which would imply that functions like abs should just use HLSL_Availability instead and we can do away with HLSL_16bit_AVAILABILITY. Since you want all function overloads with a half parameter to always be exposed.

@llvm-beanz Thoughts on this?

Looking at this again' _HLSL_16BIT_AVAILABILITY' is defined even if _HLSL_ENABLE_16_BIT isn't defined; the important thing is the overloads are defined even if there is no native half.

Copy link
Contributor

@bob80905 bob80905 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked offline, LGTM

@bob80905
Copy link
Contributor

this is not NFC, so we should verify that we can call these intrinsics with half values even if 16-bit types aren't enabled, and that they properly codegen to 32-bit varia

For example, for abs, it still depends on the _HLSL_16BIT_AVAILABILITY availability attribute. Does this PR intend to keep abs overloads using half "unexposed"? Or should that overload for abs be exposed too?

Oh I see what you are referring to. That might be my mistake; let me double check if I used the wrong __HLSL_AVAILABILITY

Yeah, if the intention of this PR is to expose all half type overloads always, then I would think there is no more utility in defining HLSL_16BIT_AVAILABILITY anymore.

I think I was meant to use _HLSL_16BIT_AVAILABILITY and not _HLSL_AVAILABILITY; I'll fix that. Thanks for noticing the error!

What I'm trying to get at is, you're removing #ifdef __HLSL_ENABLE_16_BIT, and replacing it with an availability attribute making sure that the shader model is at least 6.2 for these functions. If you look at the definition for HLSL_16bit_availability, you'll notice that it's also defined under an ifdef: #ifdef __HLSL_ENABLE_16_BIT The essence of this PR seems to me like it's assuming from now on that #ifdef __HLSL_ENABLE_16_BIT will always be true. So, that means this availability attribute will always be defined. And it's identical to the HLSL_AVAILABILITY attribute. I might understand this incorrectly, but now it seems to me like there's no point or distinguishing use between HLSL_AVAILABILITY and HLSL_16bit_AVAILABILITY. Does that make sense? Which would imply that functions like abs should just use HLSL_Availability instead and we can do away with HLSL_16bit_AVAILABILITY. Since you want all function overloads with a half parameter to always be exposed.

@llvm-beanz Thoughts on this?

Looking at this again' _HLSL_16BIT_AVAILABILITY' is defined even if _HLSL_ENABLE_16_BIT isn't defined; the important thing is the overloads are defined even if there is no native half.

Nevermind I missed the #else :P
All resolved.

@spall spall merged commit 14d2561 into llvm:main Mar 24, 2025
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HLSL] half overloads and typedefs are supported when 16-bit types are not enabled for some overloads
5 participants