[HLSL] Reapply Move length support out of the DirectX Backend (#121611) #122337
Conversation
farzonl
commented
Jan 9, 2025
farzonl
commented
- attribute keeps getting deleted on bad rebases
llvmbot
added
clang
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-backend-x86 Author: Farzon Lotfi (farzonl) Changes
Full diff: https://github.com/llvm/llvm-project/pull/122337.diff 1 Files Affected:
diff --git a/clang/lib/Headers/hlsl/hlsl_intrinsics.h b/clang/lib/Headers/hlsl/hlsl_intrinsics.h
index cf287e598f76ba..7105d83078a9d7 100644
--- a/clang/lib/Headers/hlsl/hlsl_intrinsics.h
+++ b/clang/lib/Headers/hlsl/hlsl_intrinsics.h
@@ -1297,9 +1297,11 @@ float4 lerp(float4, float4, float4);
///
/// Length is based on the following formula: sqrt(x[0]^2 + x[1]^2 + ...).
+_HLSL_16BIT_AVAILABILITY(shadermodel, 6.2)
const inline half length(half X) { return __detail::length_impl(X); }
const inline float length(float X) { return __detail::length_impl(X); }
+_HLSL_16BIT_AVAILABILITY(shadermodel, 6.2)
template <int N> const inline half length(vector<half, N> X) {
return __detail::length_vec_impl(X);
}
|
|
@llvm/pr-subscribers-clang Author: Farzon Lotfi (farzonl) Changes
Full diff: https://github.com/llvm/llvm-project/pull/122337.diff 1 Files Affected:
diff --git a/clang/lib/Headers/hlsl/hlsl_intrinsics.h b/clang/lib/Headers/hlsl/hlsl_intrinsics.h
index cf287e598f76ba..7105d83078a9d7 100644
--- a/clang/lib/Headers/hlsl/hlsl_intrinsics.h
+++ b/clang/lib/Headers/hlsl/hlsl_intrinsics.h
@@ -1297,9 +1297,11 @@ float4 lerp(float4, float4, float4);
///
/// Length is based on the following formula: sqrt(x[0]^2 + x[1]^2 + ...).
+_HLSL_16BIT_AVAILABILITY(shadermodel, 6.2)
const inline half length(half X) { return __detail::length_impl(X); }
const inline float length(float X) { return __detail::length_impl(X); }
+_HLSL_16BIT_AVAILABILITY(shadermodel, 6.2)
template <int N> const inline half length(vector<half, N> X) {
return __detail::length_vec_impl(X);
}
|
| @@ -1297,9 +1297,11 @@ float4 lerp(float4, float4, float4); | |||
| /// | |||
| /// Length is based on the following formula: sqrt(x[0]^2 + x[1]^2 + ...). | |||
|
|
|||
| _HLSL_16BIT_AVAILABILITY(shadermodel, 6.2) | |||
| const inline half length(half X) { return __detail::length_impl(X); } | |||
| const inline float length(float X) { return __detail::length_impl(X); } | |||
There was a problem hiding this comment.
Do we need _HLSL_AVAILABILITY(shadermodel, 6.2) on the none half variants?
No failures. We would need to have tests for all intrinsics that if its not SM 6.2 or greater uses of half would error, but we never did that for any of the other intrinsics so we should probably discuss what the test coverage should be and apply it broadly. |
## Changes - Delete DirectX length intrinsic - Delete HLSL length lang builtin - Implement length algorithm entirely in the header. ## History - In the past if an HLSL intrinsic lowered to either a spirv op code or a DXIL opcode we represented it with intrinsics ## Why we are moving away? - To make HLSL apis more portable the team decided that it makes sense for some intrinsics to be defined only in the header. - Since there tends to be more SPIRV opcodes than DXIL opcodes the plan is to support SPIRV opcodes either with target specific builtins or via pattern matching.
farzonl
force-pushed
the
fix-length-half-availability
branch
from
0f0b75d to
6f2da80
Compare
llvmbot
added
clang:frontend
farzonl
changed the title
farzonl
changed the title
- attribute keeps getting deleted on bad rebases
farzonl
force-pushed
the
fix-length-half-availability
branch
from
6f2da80 to
7173a48
Compare
You can test this locally with the following command:git-clang-format --diff d01ae567742c4d83b67483e15eb74c0ecd2e8270 7173a486d82a3959fc6157a58488b10ece492821 --extensions cpp,h -- clang/lib/CodeGen/CGBuiltin.cpp clang/lib/CodeGen/CGHLSLRuntime.h clang/lib/Headers/hlsl/hlsl_detail.h clang/lib/Headers/hlsl/hlsl_intrinsics.h clang/lib/Sema/SemaHLSL.cpp llvm/lib/Target/DirectX/DXILIntrinsicExpansion.cppView the diff from clang-format here.diff --git a/clang/lib/Headers/hlsl/hlsl_intrinsics.h b/clang/lib/Headers/hlsl/hlsl_intrinsics.h
index f4470d5de6..e5ff7531d2 100644
--- a/clang/lib/Headers/hlsl/hlsl_intrinsics.h
+++ b/clang/lib/Headers/hlsl/hlsl_intrinsics.h
@@ -1301,7 +1301,7 @@ _HLSL_16BIT_AVAILABILITY(shadermodel, 6.2)
const inline half length(half X) { return __detail::length_impl(X); }
const inline float length(float X) { return __detail::length_impl(X); }
-template <int N>
+template <int N>
_HLSL_16BIT_AVAILABILITY(shadermodel, 6.2)
const inline half length(vector<half, N> X) {
return __detail::length_vec_impl(X);
|
In practice these are pretty tricky to test on just Clang because the 6.2 attribute only applies when -enable-16bit-types is set, which requires shader model 6.2 or greater. So you shouldn't actually be able to make the compiler fail this. The point of the attribute is really just for header completeness so that we can drive documentation and LSP tooling from it. |
|
@llvm-beanz I'm really confused what is going on here
BUT its failing in buildkite and on other llvm builders. I'm not sure what could do this. |
