microsoft / DirectXShaderCompiler

This repo hosts the source for the DirectX Shader Compiler which is based on LLVM/Clang.
Other
3.04k stars 677 forks source link

Unspecified behavior from new-to-DXIL sdiv instruction #3150

Open jenatali opened 3 years ago

jenatali commented 3 years ago

DXBC had a udiv instruction, with clearly-spec'd behavior for corner cases like divide by 0 (should produce maximum uint).

DXIL introduces an sdiv instruction, but doesn't specify what should happen with:

llvm-beanz commented 1 year ago

I assume division by zero is undefined in HLSL as it is in C/C++. If not, it probably should be (and effectively is now).

I think INT_MIN / -1 overflows and is thus also undefined behavior.

LLVM IR and SPIR-V both specify that division by zero is undefined:

My take here is that we should document this behavior, but not change anything.

@tex3d, @pow2clk any thoughts here?

pow2clk commented 11 months ago

At least one difference between what DXIL.rst says and the LLVM spec is the behavior of divide by zero for udiv. DXIL says it should be 0xFFFFFFFF, LLVM says it's undefined.

We should determine what existing hardware does when they encounter udiv. If everyone is already conforming to the DXIL spec, we can keep that behavior. Otherwise, we could switch to LLVM. Same for idiv.

damyanp commented 2 months ago

@jenatali - is the dxbc converter generating idiv instructions (we can't find one, so presumably this means sdiv?) Or is the dxbc generating the dxil udiv operation (as opposed to the udiv LLVM IR instruction)?

jenatali commented 2 months ago

Ah apparently I had the wrong name, it's sdiv, not idiv. I'm not seeing this from the DXBC converter, but it's easy to hit: https://godbolt.org/z/bnTWGecr4

Now that you brought it up, I do actually see that the DXBC converter generates the udiv operation, as opposed to the udiv LLVM instruction, which means that those probably can/do have different semantics.

llvm-beanz commented 2 months ago

I've written a test to figure out what production drivers actually do for this over in my fancy WoL test framework:

https://github.com/llvm-beanz/HLSLTest/blob/main/test/Basic/idiv.yaml

What I've discovered is that through lack of testing we've basically inherited the undefined behavior here.

My test asserts that: 1/0 = -1 -1/0 = -1 INT_MAX / 0 = -1 INT_MIN / 0 = -1 0/0 = -1

1/-1 = -1 -1/-1 = 1 INT_MAX / -1 = INT_MIN + 1 INT_MIN / -1 = INT_MIN 0/-1 = 0

I've tested this on NVIDIA DirectX, NVIDIA Vulkan, Intel DirectX, Intel Vulkan and Apple. All tested platforms handle division by -1 the same.

Division by zero has different behavior on different drivers. The NVIDIA DirectX driver, and both NVIDIA and Intel Vulkan drivers return -1 for all division by zero. Apple returns -1 for all cases except 0/0 which it returns 0 for. The Intel DirectX driver returns INT_MAX when the numerator is >= 0 and INT_MIN when the numerator is < 0.

llvm-beanz commented 2 months ago

I guess one other thing worth noting is that LLVM constant evaluates division-by-zero to undef if both the numerator and denominator are constants. If just the denominator is constant it leaves the instruction unevaluated (which is odd to me): https://godbolt.org/z/Ef6bqb9es

damyanp commented 2 months ago

Plan: add a note in DXIL.rst that sdiv divide by zero behavior is undefined.