[Feature Request] Ability to use [vk::ext_reference] attribute on any function (not just inline SPIR-V) argument to indicate to treat is a reference

[devshgraphicsprogramming]

I'm unsure whether this should be opened here or in https://github.com/microsoft/hlsl-specs because the attribute already exists and can be applied to any function argument without the compiler complaining (no diagnostics emitted).

Is your feature request related to a problem? Please describe.

Unless something changed w.r.t vk::ext_reference in microsoft/hlsl-specs#59 then after the merge of #5249 it will become impossible for me to implement certain load types and atomics if I want to "default" certain OpCode operands, for example.

template<typename T>
[[vk::ext_instruction(/* OpLoad */ 61)]]
T special_load([[vk::ext_reference]] T arg, [[vk::ext_literal]] int aligned, , [[vk::ext_literal]] int alignment, [[vk::ext_literal]] int memoryOperand);

template<typename T,>
T volatile_load(inout T myref)
{
   return special_load(myref,/*Aligned*/0x2,Alignment,/*Volatile*/0x1);
}

Currently it works beautifully on RWStructuredBuffer( https://godbolt.org/z/1j3hczKcT ), groupshared ( https://godbolt.org/z/9qso3Y7eW ) and might even work with the new current implementation of BDA with vk::BufferPointer<T,A>::Get().

However this relies on inout working as a reference, which is a bug (is contrary to the HLSL spec, and this will be fixed in #5249).

One that behaviour of inout goes away, the only way to achieve this is to replace any function in the call chain with a Preprocessor Macro Function which copy-pastes a scope with block of code. This brings us back to the feature parity with GLSL as:

• the macro cannot be in a namespace
• the macro is a free floating function (you cannot make it into a struct method)
• the macro cannot return a value, has to behave like a void function
• now every argument/parameter is a reference
• no ability to use return within the macro (would need to wrap it in a weird do-while(false) and use continue instead of return)
• possible variable shadowing as its just code copy-paste

TL;DR Its possible to work around, but leads to completely unmaintainable code.

This is especially important for things like atomic operations and atomic load/store which depending on the Vulkan environment you're targetting (with or without Vulkan Memory Model) you'd want to differentiate and "hide"/default some of the ext_literal operands.

Other DXC users have ran into this before ( #5077 ) when attempting to shim GLSL-compatible code, albeit without SPIR-V intrinsics like I'm doing.

Describe the solution you'd like

The ability to mark arguments in the the non-intrinsic functions (i.e. volatile_load and `` in the examples above).

Describe alternatives you've considered

Waiting for HLSL proposal 0006 to go under review, get approved, implemented in DXC.

Additional context

Currently inout works like a reference "by accident" but this behaviour will go away after #5377 gets merged.

Actually Alexander Sannikov @Raikiri (working on Path of Exile) mentioned to me that they were "considering writing our code generator that automatically inlines all our shader functions before compiling them, just to avoid relying on inout being sane" and his definition of "sane" was for inout to act as a reference.

However as we know both HLSL and GLSL use "value return" calling conventions and inout is specified to be exactly "copy-in copy-out".

From my limited knowledge and investigations, it does seem that as long as spirv-opt and DXC's own DXIL opimizers work properly and after optimization (inlining and temporary store elimination) a true reference vs "copy-in copy-out" shouldn't differ at all in code emitted... unless we're dealing with temporal effects like volatile reads/writes and atomics.

However my example & problem clearly demonstrate the need for references and atomic/volatile access.

This touches microsoft/hlsl-specs#83

[s-perron]

Sounds good we will try to not remove this functionality. To make sure we do not, we need to add a test. Once we add a test, we can consider this closed.

[s-perron]

Below is a full test for what I expect is wanted. It does not currently pass. The vk::ext_reference is ignored on the call to SequentiallyConsistentAtomicIncrement, so the atomic operation ends up being on the temporary that is created. I can see why this is desirable.

However, this is out of scope for what was expected from vk::ext_reference originally. The fact that it was silently ignored instead of giving an error is more of an oversight.

From the initial description, I thought that vk::ext_reference was already doing what we wanted it to do, which is why I suggested adding a test. I would accept a patch that make this work the way that @devshgraphicsprogramming wants it to, but it is not a priority for the maintainers at this time.

// RUN: %dxc -T cs_6_0 -E main  %s -spirv  2>&1 | FileCheck %s

[[vk::ext_instruction(/* OpAtomicIIncrement */ 232)]]
int AtomicIncrement([[vk::ext_reference]] int arg, int scope, int memory_semantics);

int SequentiallyConsistentAtomicIncrement( [[vk::ext_reference]] int pointer) {   
  return AtomicIncrement(pointer, 1, 0x10);
}

struct BufferType {
    int     a;
};

RWStructuredBuffer<BufferType> sbuf;

// CHECK: [[sbuf:%[a-zA-Z0-9_]+]] = OpVariable %_ptr_Uniform_type_RWStructuredBuffer_BufferType Uniform                                      
// CHECK:  %main = OpFunction                                                                                                                
// CHECK: [[ac:%[0-9]+]] = OpAccessChain %_ptr_Uniform_int [[sbuf]] %int_0 %uint_0 %int_0                                                    
// CHECK: OpAtomicIIncrement %int [[ac]] %int_1 %int_16                                                                                      
[numthreads(8, 4, 1)]                                                                                                                        
void main() {                                                                                                                                
  SequentiallyConsistentAtomicIncrement(sbuf[0].a);                                                                                          
}  

[devshgraphicsprogramming]

Below is a full test for what I expect is wanted. It does not currently pass. The vk::ext_reference is ignored on the call to SequentiallyConsistentAtomicIncrement, so the atomic operation ends up being on the temporary that is created. I can see why this is desirable.

However, this is out of scope for what was expected from vk::ext_reference originally. The fact that it was silently ignored instead of giving an error is more of an oversight.

From the initial description, I thought that vk::ext_reference was already doing what we wanted it to do, which is why I suggested adding a test. I would accept a patch that make this work the way that @devshgraphicsprogramming wants it to, but it is not a priority for the maintainers at this time.

// RUN: %dxc -T cs_6_0 -E main  %s -spirv  2>&1 | FileCheck %s

[[vk::ext_instruction(/* OpAtomicIIncrement */ 232)]]
int AtomicIncrement([[vk::ext_reference]] int arg, int scope, int memory_semantics);

int SequentiallyConsistentAtomicIncrement( [[vk::ext_reference]] int pointer) {   
  return AtomicIncrement(pointer, 1, 0x10);
}

struct BufferType {
    int     a;
};

RWStructuredBuffer<BufferType> sbuf;

// CHECK: [[sbuf:%[a-zA-Z0-9_]+]] = OpVariable %_ptr_Uniform_type_RWStructuredBuffer_BufferType Uniform                                      
// CHECK:  %main = OpFunction                                                                                                                
// CHECK: [[ac:%[0-9]+]] = OpAccessChain %_ptr_Uniform_int [[sbuf]] %int_0 %uint_0 %int_0                                                    
// CHECK: OpAtomicIIncrement %int [[ac]] %int_1 %int_16                                                                                      
[numthreads(8, 4, 1)]                                                                                                                        
void main() {                                                                                                                                
  SequentiallyConsistentAtomicIncrement(sbuf[0].a);                                                                                          
}  

we can assign someone and implement it

[devshgraphicsprogramming]

Actually there's another thing I noticed I can do which is pretty nasty

struct PSInput
{
    float4 position : SV_Position;
    float4 color    : COLOR0;
};

namespace impl
{
template<typename T>
struct declval
{
    static declval<T> create()
    {
        return declval<T>();
    }
    // must be an array, otherwise __decltype doesn't turn into a reference!
    T member[1];
};
}

template<typename T>
struct add_lvalue_reference
{
    using type = __decltype(impl::declval<T>::create().member[0]);
};

void SetRef(add_lvalue_reference<float>::type t)
{
    t = 9.f;
}

float4 PSMain(PSInput input) : SV_Target0
{
    float b = 6.f;
    SetRef(b);
    return input.color * float(b);
}

Neither SPIR-V or DXIL output know what to do with a reference type though.

But its useful enough to template on (type_traits), so things like alignment_of<T>::value work nicely.
I just need to remember to not attempt to use a reference type as a variable in an evaluated context.

[devshgraphicsprogramming]

I'm actualy trying to implement this with Proposal 0011 #6578

[devshgraphicsprogramming]

I'm going to close this, but I'd appreciate #6578 getting reopened because there's no way to "link up" the inline SPIR-V vk::SpirvType like BDA, or CombinedSamplerImages that need to refer to a HLSL "regular" type.

#6578 (comment)

A vk::__spirvtype(type) to obtain a symbolic %result_id of a declaration HLSL codegen will create in the future would be indispensable.

And because OpBitcast shouldn't be used to cast a pointer to the same type (not legal SPIR-V the types need to be different as per the spec) an intrinsic to get vk::SpirvType<> refof([[vk::ext_reference]] T) is necessary to link up inline spir-v code properly, vk::ext_reference isn't enough for some usages (BDA emulation, manual OpAccessChains)

Note: a vk::__spirvtype(expr) might be necessary because a type T declated in HLSL may exist in the SPIR-V codegen in multiple versions depending on member decorations and storage class.