An issue with template type deduction and globallycoherent?

[simonwongms]

We have a variable of type ‘globallycoherent RWByteAddressBuffer’. When this is passed to a templated function where the buffer is a templated parameter, the globallycoherent part seems to be missed?
Now, I don’t know if the compiled out dxil is actually correct and the warning/error is a false positive. We hadn’t noticed any visual artifacts in the June GDK.

I’ve distilled the hlsl into bare bones to repro this issue. This is obviously not a shader that does anything. It’s just stripped down to illustrate the issue and trigger the error/warning.

I am able to reproduce this in shader playground:
https://shader-playground.timjones.io/98b958c232fe4b88326d5be6b2570013

The error output there matches the ones I get in the GDK

Environment
dxcompiler_xs.dll!DxcCreateInstance: 1.7 - 2310.2310.11301.10016

[simonwongms]

Just wanted to check on the status of this. Thanks

[KStocky]

+1 For this. Another example in shader playground https://shader-playground.timjones.io/b050891fa1644437bf69e0ccc655801f. I would expect "globallycoherent RWBuffer" to be a different type from "RWBuffer" considering how you can't assign a globallycoherent RWBuffer to a regular RWBuffer.

[simonwongms]

Good find. Your example would probably help narrow down the source of the compiler's confusion. It looks like a different issue on the surface, but probably points to something related. eg something(s) does not recognize 'globallycoherent' as a keyword.

[llvm-beanz]

Compiler Explorer Link (because shader playground is really slow to load).

[pow2clk]

Thanks Chris, I was about to send the same link. Also worth noting, this link uses the trunk compiler which has made the warning message more helpful.

In this, a warning is produced, which isn't explicitly said in this bug. I know @simonwongms said that they didn't know if the codegen was right. Given that this example passes a globallycoherent object into a parameter that isn't, I expect the qualifier is getting stripped. I think that's correct. If there are codegen issues elsewhere, we'll need an example to consider them.

The test @KStocky added is appreciated and we should incorporate some testing for this. Otherwise, I don't see any action needed here. Am I missing something?

[llvm-beanz]

I think @KStocky's example here shows how this can become an error: https://godbolt.org/z/sP6rKvYov

We do need to figure out how we want to resolve this properly. This especially becomes a problem if we support cross-translation unit linking. In that scenario we'll need globallycoherent to be retained across call boundaries.

I think we get lucky today that the codegen is correct in most cases because we just propagate globallycoherent when the resources all get resolved back to a single underlying resource. IIUC, the point of the warning was originally to notify users when a non-globallycoherent resource becomes globallycoherent because somewhere in the usage chain it gains the property. If we continue working on supporting shader linking, this property will be much more important to act as a proper type qualifier.

[simonwongms]

Hi, Greg, you mentioned: "Given that this example passes a globallycoherent object into a parameter that isn't, I expect the qualifier is getting stripped. I think that's correct. If there are codegen issues elsewhere, we'll need an example to consider them." That's not exactly the issue here though. It's not as if we have a function that is explicitly written to expect a non globallycoherent RWBuffer, and the calling code is passing one. The bug here is that the function is a templated function, and the type should be getting auto-deduced to be a globallycoherent RWBuffer. In which case, it should maintain the qualifier. When I dug into the disasm in PIX, I can see that the generated code does seem correct. For example, this loading code template uint TemplateFunctionR(BufferType AutoParam) { return AutoParam.Load(1); } generates this: buffer_load_dword v0, v0, s[0:3], 0 offset:1 dlc glc // 000000000030: E030C001 80000000 So it does seem to generate the correct code in this example, which would mean that the error/warning is a false positive. (Edited by @llvm-beanz to remove email thread text)

[llvm-beanz]

So it does seem to generate the correct code in this example, which would mean that the error/warning is a false positive.

The warning isn't a false positive. The code is converting between globallycoherent and non-globallycoherent resource types. This causes DXC to propagate the globallycoherent annotation to the non-globallycoherent resources. While that does make the code "correct" it also means you're getting extra memory barriers and syncs, which the compiler can't tell if you actually intended. Hence the warning.

The problem is that because of how globallycoherent was implemented the compiler drops the annotation when resolving overloads and template instantiations (which is a bug that is hard to fix).

We've had a bunch of issues relating to type annotations like globallycoherent interacting with templates, and with HLSL library shaders. Resolving those is going to likely require some re-design of globallycoherent and will probably require breaking changes to the language so we'd need to track it as a feature for a new language version.

[KStocky]

We've had a bunch of issues relating to type annotations like globallycoherent interacting with templates, and with HLSL library shaders.

Would this be the same bug as something like row_major for example. It is also a type qualifier that does not seem to participate in template type resolution.

[llvm-beanz]

We've had a bunch of issues relating to type annotations like globallycoherent interacting with templates, and with HLSL library shaders.

Would this be the same bug as something like row_major for example. It is also a type qualifier that does not seem to participate in template type resolution.

Yes, same basic problem different attribute. The matrix orientation and globallycoherent keywords are implemented as attributes, so whenever we need a "canonical type" we lose them. Canonical type resolutions occur in a bunch of places whenever the compiler needs to de-sugar or fully resolve a type.

We really need these keywords to be implemented as type qualifiers (which in clang are effectively just special attributes, that stick around during canonicalization).