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

Ambiguous overload resolution chooses the wrong option with no warning #6938

Closed
apanteleev opened this issue Sep 27, 2024 · 3 comments
Closed
Labels
bug Bug, regression, crash needs-triage Awaiting triage

Comments

@apanteleev
Copy link
Contributor

Description
Consider the following test shader:

RWBuffer<int> buf;

[numthreads(1,1,1)]
void main()
{
    int i = buf[0];
    uint u = 345;
    i = clamp(i, 0, u);
    buf[0] = i;
}

Here, one would expect that i is clamped to be between 0 and the value of u, making sure that negative values do not pass through. Instead, DXC chooses the version of clamp that works on uint, casts i to uint before clamping which makes the comparison with 0 useless, and then casts the result back to int. Adding a cast int(u) makes it choose the right (signed) version of clamp.

It's unclear to me if this is expected behavior from the HLSL point of view, but it would be great if we got a warning in such ambiguous (and unsafe) cases.

Steps to Reproduce
Compile the shader above with any CS profile.

Actual Behavior
DXIL output - note the UMin operation and lack of UMax which is optimized away.

define void @main() {
  %1 = call %dx.types.Handle @dx.op.createHandle(i32 57, i8 1, i32 0, i32 0, i1 false)  ; CreateHandle(resourceClass,rangeId,index,nonUniformIndex)
  %2 = call %dx.types.ResRet.i32 @dx.op.bufferLoad.i32(i32 68, %dx.types.Handle %1, i32 0, i32 undef)  ; BufferLoad(srv,index,wot)
  %3 = extractvalue %dx.types.ResRet.i32 %2, 0
  %4 = call i32 @dx.op.binary.i32(i32 40, i32 %3, i32 345)  ; UMin(a,b)
  call void @dx.op.bufferStore.i32(i32 69, %dx.types.Handle %1, i32 0, i32 undef, i32 %4, i32 %4, i32 %4, i32 %4, i8 15)  ; BufferStore(uav,coord0,coord1,value0,value1,value2,value3,mask)
  ret void
}

SPIRV output:

...
         %14 = OpLoad %type_buffer_image %buf
         %15 = OpImageRead %v4int %14 %uint_0 None
         %16 = OpCompositeExtract %int %15 0
         %17 = OpBitcast %uint %16
         %18 = OpExtInst %uint %1 UClamp %17 %uint_0 %uint_345
         %19 = OpBitcast %int %18
         %20 = OpLoad %type_buffer_image %buf
               OpImageWrite %20 %uint_0 %19 None

Environment

  • DXC version libdxcompiler.so: 1.8(dev;4746-75ff50ca); libdxil.so: 1.8
  • Host Operating System - Debian 12
@apanteleev apanteleev added bug Bug, regression, crash needs-triage Awaiting triage labels Sep 27, 2024
@llvm-beanz
Copy link
Collaborator

DXC has some pretty special logic for overload resolution, which is further complicated by treating overload resolution of standard library functions different from user-defined functions. We can't warn on "ambiguous" overloads for two reasons (1) DXC doesn't think they're ambiguous, and (2) it would likely be extremely noisy because a lot of code depends on DXC's odd overload resolution behavior.

What we can do is warn on the parameter conversions, and if you pass -Wconversion to the compiler you will get parameter conversion warnings.

Changing the overload resolution algorithm for HLSL is planned and implemented in Clang. Clang treats the HLSL standard library functions as if they were any other function, and uses a best match algorithm that aligns more closely to C++. In Clang this code will fail to compile as an ambiguous overload.

Here is a Compiler Explorer link demonstrating the warnings from DXC and the error from Clang.

Given the context above I don't see any further actionable change for DXC, and the long-term fix is already implemented in Clang. I'm going to close this issue as not planned since we have no plans to make changes to DXC.

@damyanp damyanp closed this as not planned Won't fix, can't repro, duplicate, stale Oct 1, 2024
@github-project-automation github-project-automation bot moved this to Triaged in HLSL Triage Oct 1, 2024
@apanteleev
Copy link
Contributor Author

Thanks @llvm-beanz, -Wconversion is useful advice.

Is there a list of all such warning settings for DXC somewhere, or is it identical to Clang? I only found DiagnosticGroups.td.

@llvm-beanz
Copy link
Collaborator

llvm-beanz commented Oct 1, 2024

DXC is a fork of Clang 3.7. Many, but not all of the diagnostics supported by Clang 3.7 are supported by clang. There are additional diagnostics that DXC has added for HLSL. That file does list all the diagnostic groups supported in DXC, but we have no comprehensive documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug, regression, crash needs-triage Awaiting triage
Projects
Archived in project
Development

No branches or pull requests

3 participants