Skip to content

[HLSL] Overload resolution should promote 16-bit types #81047

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
Tracked by #67692
llvm-beanz opened this issue Feb 7, 2024 · 1 comment · Fixed by #90222
Closed
Tracked by #67692

[HLSL] Overload resolution should promote 16-bit types #81047

llvm-beanz opened this issue Feb 7, 2024 · 1 comment · Fixed by #90222
Assignees
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" HLSL HLSL Language Support

Comments

@llvm-beanz
Copy link
Collaborator

llvm-beanz commented Feb 7, 2024

In working on #71098, I've discovered a few cases where C++ overload resolution rules don't quite match HLSL. HLSL allows "promotion" casting 16-bit types to larger types, and will select the first larger type that fits. In the following examples both should resolve successfully to the 32-bit overloads:

void Fn3(double2 D);
void Fn3(float2 F);

void Call3(half2 H) {
  Fn3(H);
}

void Fn4(int64_t2 L);
void Fn4(int2 I);

void Call4(int16_t H) {
  Fn4(H);
}

Notes:

  • The half support is currently incorrect
  • We do not correctly disable int 16bit types

Acceptance Criteria

  • A test case demonstrating the above overload resolution.
  • Spec definition around promotion of types is created.
    • Spec update link to be provided in the PR for this item
  • A best effort list of differences between LLVM and DXC overloads is represented in tests and in the DXC vs clang document
@llvm-beanz llvm-beanz converted this from a draft issue Feb 7, 2024
@llvm-beanz llvm-beanz self-assigned this Feb 7, 2024
@llvm-beanz llvm-beanz added the HLSL HLSL Language Support label Feb 7, 2024
@llvm-beanz llvm-beanz changed the title Overload resolution should promote 16-bit types [HLSL] Overload resolution should promote 16-bit types Feb 7, 2024
llvm-beanz added a commit to llvm-beanz/llvm-project that referenced this issue Feb 7, 2024
This addresses feedback from @rjmcall. The main updates are:
* Added asserts and todos about handling constrained intrinsics.
* Fixed sufflevector code generation (and updated tests)
* Fixed shadowed promotion casts. Writing a test for this revealed
another issue.
* Added Element conversion to conversion rank checking, and added test
to verify promotion is preferred over conversion.
* Added HLSL integral promotion check.

There are still at least two outstanding bugs that are exposed by this
change which I've added an XFAIL'd test for (llvm#81047 & llvm#81049).

HLSL's promotion rules are not well written down, but specifically in
overload resolution integer and floating point values can implicitly
cast to larger types and the smallest of the larger types is the best
match.
@llvm-beanz llvm-beanz removed their assignment Mar 11, 2024
@llvm-beanz llvm-beanz self-assigned this Apr 5, 2024
@damyanp damyanp moved this to Planning in HLSL Support Apr 8, 2024
@damyanp damyanp moved this from Planning to Active in HLSL Support Apr 23, 2024
llvm-beanz added a commit to llvm-beanz/llvm-project that referenced this issue Apr 26, 2024
This PR fixes bugs in HLSL floating conversions. HLSL always has
`half`, `float` and `double` types, which promote in the order:

`half`->`float`->`double`

and convert in the order:

`double`->`float`->`half`

As with other conversions in C++, promotions are preferred over
conversions.

We do have floating conversions documented in the draft language
specification (https://microsoft.github.io/hlsl-specs/specs/hlsl.pdf
[Conv.rank.float]) although the exact language is still in flux
(microsoft/hlsl-specs#206).

Resolves llvm#81047
llvm-beanz added a commit that referenced this issue May 2, 2024
This PR fixes bugs in HLSL floating conversions. HLSL always has `half`,
`float` and `double` types, which promote in the order:

`half`->`float`->`double`

and convert in the order:

`double`->`float`->`half`

As with other conversions in C++, promotions are preferred over
conversions.

We do have floating conversions documented in the draft language
specification (https://microsoft.github.io/hlsl-specs/specs/hlsl.pdf
[Conv.rank.float]) although the exact language is still in flux
(microsoft/hlsl-specs#206).

Resolves #81047
@github-project-automation github-project-automation bot moved this from Active to Done in HLSL Support May 2, 2024
@EugeneZelenko EugeneZelenko added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label May 2, 2024
@llvmbot
Copy link
Member

llvmbot commented May 2, 2024

@llvm/issue-subscribers-clang-frontend

Author: Chris B (llvm-beanz)

In working on #71098, I've discovered a few cases where C++ overload resolution rules don't quite match HLSL. HLSL allows "promotion" casting 16-bit types to larger types, and will select the first larger type that fits. In the following examples both should resolve successfully to the 32-bit overloads:
void Fn3(double2 D);
void Fn3(float2 F);

void Call3(half2 H) {
  Fn3(H);
}

void Fn4(int64_t2 L);
void Fn4(int2 I);

void Call4(int16_t H) {
  Fn4(H);
}

Notes:

  • The half support is currently incorrect
  • We do not correctly disable int 16bit types

Acceptance Criteria

  • A test case demonstrating the above overload resolution.
  • Spec definition around promotion of types is created.
    • Spec update link to be provided in the PR for this item
  • A best effort list of differences between LLVM and DXC overloads is represented in tests and in the DXC vs clang document

@damyanp damyanp moved this to Closed in HLSL Support Apr 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" HLSL HLSL Language Support
Projects
Status: Closed
Development

Successfully merging a pull request may close this issue.

3 participants