-
Notifications
You must be signed in to change notification settings - Fork 36
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
base: main
Are you sure you want to change the base?
Report on impact of overload resolution behavior changes #408
Conversation
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approach, the folllowing
There was a problem hiding this 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?
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. |
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.
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? |
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. |
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.