Skip to content
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

Report on impact of overload resolution behavior changes #408

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

llvm-beanz
Copy link
Collaborator

The HLSL draft specification aligns HLSL's overload resolution rules with C++ overload resolution rules. This will have an impact on source compatibility for shaders migrating from DXC to Clang.

This report analyzes the problem space, includes details about experimentation migrating an existing HLSL codebase to Clang, and suggests several possible options for mitigating the problem as well identifying a preferred approach to be implemented in Clang and reflected in the HLSL 202x language specification.

This documents the observed impact of overload resolution changes taken
into the HLSL specification and Clang implementation, and proposes
strategies for mitigation of code migration challenges.

The second part of DXC's approach is how DXC selects truncated overload sets for
standard library functions. DXC processes overload candidates for intrisncs
outside the overload sets, and only puts one matching overload candidate in
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does it pick which one to add?

user-defined functions is a source of confusion and bugs.

DXC's custom overload resolution also does not handle cases that can arise in
HLSL like constant implicit objects in member functions, non-member overloaded
Copy link
Collaborator

Choose a reason for hiding this comment

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

HLSL, like
*i think a comma is needed here.

DXC will resolve the type as `float` when integer types are provided. Some
examples of such operations are `rcp`, `exp`, `pow` and `sqrt`. None of these
operations really make sense on integers, so it is unlikely that the user
expected integer lower.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know what 'expected integer lower' means so I'm unsure if this is what you meant to type.

### Implicitly resolved; potentially bad results

Some math operations where DXC matches intrinsics come with significant
performance impacts. For example consider the `lerp` intrinsic which only has
Copy link
Collaborator

Choose a reason for hiding this comment

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

what are the performance impacts?

Any solution should involve adding missing overloads that DXC lowers to valid
and efficient IR. The absence of those overloads should be viewed as a bug in
Clang. The following possible solutions should be viewed as applying only to
cases where DXC resolves overloads through implicit conversion of arguments to
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sentence is very confusing to me.

* The third category of overloads should be left out from Clang and produce
errors when migrating from DXC to Clang.

As the recommended approach the following issues have been filed:
Copy link
Collaborator

Choose a reason for hiding this comment

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

approach, the folllowing

Copy link
Collaborator

@V-FEXrt V-FEXrt left a comment

Choose a reason for hiding this comment

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

What is the mechanical difference between a report and a proposal?

@llvm-beanz
Copy link
Collaborator Author

What is the mechanical difference between a report and a proposal?

Yea... I didn't really feel like this was a proposal since I'm not really proposing a change to the language behavior (although I guess that's slicing an odd dimension since the available overloads is part of the "standard library" definition).

We had previously (without a proposal) aligned the language specification on using a C++-like best-match algorithm, and I didn't really want to re-open previous discussions and navigate the proposal process. Maybe that was a mistake, but in that context I viewed this document more as a defect report with mitigations.

@V-FEXrt
Copy link
Collaborator

V-FEXrt commented Feb 27, 2025

We had previously (without a proposal) aligned the language specification on using a C++-like best-match algorithm

Yeah, given that I see why it would be on the fence and ultimately you just have to pick a place for it. I'm happy having it as a report I just wanted to get an understanding for why it would be one versus the other.

since the available overloads is part of the "standard library" definition

IMO the standard lib falls very clearly under the "spec of the language" but HLSL makes everything fuzzier with all the shader model specs. Do we have any code that is "standard library" but not specified by the SM?

@llvm-beanz
Copy link
Collaborator Author

Do we have any code that is "standard library" but not specified by the SM?

I think this is somewhat of a vague area that we need to work on. Ideally the "standard library" is more tied to language version than shader model. Because some of the APIs in the standard library have hardware requirements that not all targets support we'll likely need to define "required" and "optional" features in the standard library as we define it in the spec.

Shader Models also don't map cleanly to Vulkan/SPIRV, so we'll have to rectify that too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants