Atomics on RWTexture2DMS result in silent UB or ICE

[Maraneshi]

Description

The Shader Model 6.7 spec does not indicate whether atomics are supposed to work on multisampled textures when WritableMSAATexturesSupported is true. DXC does compile atomics when not explicitly specifying the sample index (e.g. InterlockedMax(tex[uv], ...)). It does not when you do specify it (e.g. InterlockedMax(tex.sample[s][uv], ...)), producing an internal compiler error.

After some investigation, the DXIL op textureStoreSample for non-atomic access was added in SM6.7 to supply the sample index (note: the parameters are not documented in DXIL.rst but it's easy enough to compare to plain textureStore), but atomicBinOp did not receive an equivalent additional variant. Thus, atomics on MS textures are currently impossible. The implicit sample 0 case that DXC does compile actually results in undefined behavior, where the resulting DXIL pushed through RGA will use an uninitialized register for the sample index instead of 0 (the hardware can actually do atomics on MS textures).

Steps to Reproduce

Godbolt link: https://hlsl.godbolt.org/z/jqG7GYfW5
Code in case it goes down (-T ps_6_7 -E PSMain):

struct PSInput {
    uint2 uv : UV;
    uint s : SAMPLE;
};

RWTexture2DMS<uint, 2> tex;

uint PSMain(PSInput input) : SV_Target0
{
    uint value = 0xDEADBEEF;
    uint old_val;

    // atomicBinOp cannot pass a sample index. RGA passes an uninitialized register (v5)!
    // This should be an error!
    // Note that for a non-MS texture RGA only passes two registers for coordinates,
    // so the hardware can actually do MS atomics (see Image Opcodes with No Sampler in ISA docs),
    // but would require a new Shader Model since atomicBinOp can't.
    InterlockedMax(tex[input.uv], value, old_val);
    
    // error: cast<X>() argument of incompatible type!
    // This should be a more user friendly error message.
    // InterlockedMax(tex.sample[input.s][input.uv], value, old_val);
    
    // --- Consistency checks, no bugs ---
    // uses textureStoreSample with sample index 0 (correct)
    tex[input.uv] = value;

    // uses textureStoreSample with sample index s (correct)
    tex.sample[input.s][input.uv] = value;
    return old_val;
}

Desired Outcome

With current DXIL these atomics are not possible, so DXC should reject them with a clear error message instead of either silent UB or an internal compiler error. (Note: Do not forget about RWTexture2DMSArray)

Environment

• DXC version: 1.10.2605.24
• Host Operating System: Windows 11

[Maraneshi]

It seems like some LLM has triaged this and made a comment that seems like half a suggestion and half a misunderstanding. atomicBinOp does not have a parameter for the sample index, so it does not "belong" anywhere and yes it would pass validation because an undefined third texture coordinate for a RWTexture2D is perfectly normal (it would have to reject it based on RWTexture2DMS not being supported rather than not putting the sample index "where it belongs"). In theory, the third texture coordinate parameter could be used as the sample index for RWTexture2DMS, but then where does it go for RWTexture2DMSArray, which already needs the third coordinate? We would need to do some bitpacking shenanigans and in either case driver compilers need to be updated to interpret all of this correctly. It is currently unclear what driver compilers actually do here and whether they even technically pull from the undef coordinate in the non-array case at all. I also don't know which hardware actually supports those atomics, it might be a different set than what currently reports SM6.7+.

All of this feels like new shader model territory rather than a "quick fix", at which point there should be a completely new DXIL instruction with an extra sample index parameter if you want to go that path instead of just rejecting the code. This is what happened with textureStore; a new textureStoreSample variant was added with an extra sample index parameter instead of forcefully shoehorning it into an existing parameter that is already used for something else, because that would just mean more silent UB or misunderstandings.

I also find it hilarious that this is listed by the bot in the list of issues it "cannot reproduce" even though it clearly can.

[damyanp]

Maraneshi - sorry for the noise. I'm experimenting with some auto-triaging tooling and although it's all meant to be done in my fork and not post anything, it tagged the issue in a commit message which is what showed up for you here. Thank you for the explanation and catching this.

[Maraneshi]

Yeah, I see now that it does have a longer explanation too that does correctly state that the atomic should be immediately rejected based on the resource type. Just the sentence about the sample index was weird/wrong and indicated not understanding what's going on. If you feel there is a better title for this issue you can of course change it.