DirectXShaderCompiler icon indicating copy to clipboard operation
DirectXShaderCompiler copied to clipboard

static variable inside cbuffer are not marked const

Open Keenuts opened this issue 1 year ago • 4 comments

Description

Found this issue while working on #6006.

Steps to Reproduce

cbuffer cbuff {
  // static = not visible to the app. Meaning not part of the constant buffer from DXIL PoV, hence why it's RW.
  static uint cbuff_a;
  uint cbuff_b;
};

[numthreads(1,1,1)]
void main()
{
  uint32_t u = cbuff_b;
  uint32_t v = cbuff_a;
  cbuff_a = 10;
  uint32_t w = cbuff_a;
}

dxc -T cs_6_0 -E main repro2.hlsl -fcgl

Actual Behavior

DXIL generates code for this, and consider cbuff_a to be a RW global variable, ignoring the fact it's contained inside a cbuffer. Don't think that's correct, I'd expect this variable to be read-only.

On the SPIR-V side, we crash, because we write to a variable contained into an uniform block.

This is a bit cursed. I would be in favor or not allowing non-const static variables into cbuffer or Push Constants. I could see how const static could end-up being used in a valid pattern.

Environment

  • DXC 136f994a2f7448200aa21e1222d96ca6c11073aa
  • Host Operating System linux

Keenuts avatar Mar 06 '24 17:03 Keenuts

https://godbolt.org/z/PWv5d7qE1

We should probably disallow static within the cbuffer. Is this something you plan to work on @keenuts?

pow2clk avatar Mar 12 '24 15:03 pow2clk

We should probably disallow static within the cbuffer

Would you still consider this illegal?

struct MyStruct {
   const static uint32_t SOME_CONSTANT = 12;
   uint32_t value;
};

cbuffer t {
    uint32_t some_value;
    MyStruct some_struct_from_the_host;
};

void main() {
    MyStruct local_struct;
    if (MyStruct::SOME_CONSTANT > local_struct.value)
        foo();
}

I could see this struct sharing between a cbuffer and local functions be useful. But I can also see that as a source of incomprehension, as you might believe SOME_CONSTANT is in the cbuffer, while it shouldn't.

Keenuts avatar Mar 12 '24 15:03 Keenuts

Upon further discussion, weird as this is, it might be intended. This is what FXC does and it is consistent with the practice of sweeping all global variables into a default cbuffer. As in many cases, the documentation could be better, but it seems the current expectation is that this behave exactly as it is. We could very well reconsider that in future language versions though.

Here is the FXC behavior (just slightly modified to remove int32_ts): https://shader-playground.timjones.io/7407ee0e5a01b6b611df41361aef5991

pow2clk avatar Mar 12 '24 15:03 pow2clk

Right.. For a static in a struct in a cbuffer, I get it. But shall we at least maybe warn for static directly inside a cbuffer (const or RW)? As this screams "mistake".

struct S {
    static uint value;
}

cbuffer t {
    static const uint value2; // warning: static variable are not considered part of the constant buffer.
    S value3; // ok, no warning.
}

Keenuts avatar Mar 14 '24 10:03 Keenuts