Skip to content

[HLSL] hlsl_intrinsics.h needs to be reorganized #129616

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

Closed
farzonl opened this issue Mar 4, 2025 · 0 comments · Fixed by #129619
Closed

[HLSL] hlsl_intrinsics.h needs to be reorganized #129616

farzonl opened this issue Mar 4, 2025 · 0 comments · Fixed by #129619
Labels
clang:headers Headers provided by Clang, e.g. for intrinsics HLSL HLSL Language Support

Comments

@farzonl
Copy link
Member

farzonl commented Mar 4, 2025

We created hlsl_intrinsics.h with the goal of having them all be alphabetically organized. When we first started implementing intrinsics in the header we noticed we might have to reorganize the file to allow some HLSL intrinsics to call other HLSL intrinsics. Our work around to avoid this was to just call the HLSL builtins. This works for most cases but does break down for the scalar cases.
Here is an example:

T frc = __builtin_hlsl_elementwise_frac(__builtin_elementwise_abs(div)); 

__builtin_hlsl_elementwise_frac will always assume the input is double for scalar cases. if if we cast it or use __builtin_fabsf or __builtin_fabsf16.

There are three potential fixes. We either move some of the intrinsics to a file that can be visible to hlsl_detail.h maybe call them hlsl_alias_intrinsics.h hlsl_core_intrinsics.horhlsl_building_block_intrinsics.h`.

or we start implementing scalar versions of our builtins that would have the type confusion problems the elementwise builtins have been causing us for scalar cases.
example:
49d584e

or we start marking hlsl builtins with custom type checking atleast for the elementwise builtins. This is the least disruptive, it is also what other elementwise builtins do, but i think we want the hlsl intrinsics written in hlsl to look like hlsl and if we are using builtins then we aren't really doing that.

IMO the most disruptive solution is probably the best long term solution.

I think @llvm-beanz agrees copied response:

I'm bought in that it is time to rethink how we organize these headers.

I think even the detail header is a bit unfortunately named, and maybe we need to think more about its structure since some of the things we're putting in there are useful language constructs that we might want elsewhere.

I can definitely see value in having all the "passthrough" intrinsics that are just mapping to builtins defined in one simple place, and having the "complex" ones broken out separately.

@farzonl farzonl added the HLSL HLSL Language Support label Mar 4, 2025
farzonl added a commit to farzonl/llvm-project that referenced this issue Mar 4, 2025
- fixes llvm#129616
- fixes llvm#129003 by adding enable_if_t vector ranges.
- alphabetize the or intrinsic
farzonl added a commit to farzonl/llvm-project that referenced this issue Mar 4, 2025
@farzonl farzonl closed this as completed in 9ee4883 Mar 5, 2025
@github-project-automation github-project-automation bot moved this to Closed in HLSL Support Mar 5, 2025
@EugeneZelenko EugeneZelenko added the clang:headers Headers provided by Clang, e.g. for intrinsics label Mar 5, 2025
jph-13 pushed a commit to jph-13/llvm-project that referenced this issue Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:headers Headers provided by Clang, e.g. for intrinsics HLSL HLSL Language Support
Projects
Status: Closed
Development

Successfully merging a pull request may close this issue.

2 participants