When is a block not a block?

[ktmattson-alion]

I was writing up an issue for OpForwardPointer... but you literally fixed it in that period 🥇
It's taken me a while to get my head around my question, so maybe this one's been resolved also 😄

So, this is more of a use-question:
If you are using EXT_buffer_reference, reference-bound buffers are decorated as Block type. If the block is used, there is a pointer declared for that type. So, if this library were intended to map these types, it would seem like the place to do it is in populate_one_ty when Pointer types are captured: see if the pointer is to a Block decorated type with PhysicalStorageBuffer as the storage class.

Ok, but I can argue to myself that Block types are not Descriptors (because they aren't) or any of the other Variable types. Should there be a Variable::Block variant?

I need this feature, so I'm going to try to work it out, but I'd rather not trip up any of the overall design if this is something you think this lib needs also. So, I'd love to hear what you think of this.

I titled this issue so that it was clear this is more of a "discuss" issue, but also because I wanted to make sure that Block decorated types which have pointers to them only occur when you're using reference buffers.

Thanks for your hard work on this useful project!

Edit: I have since answered my own silly question, as it's Block types with PhysicalStorageBuffer storage classes that exactly describe what's being captured here, although there may be other possible configurations.

[PENGUINLIONG]

thx for your attention first of all, lol. Well, Block is neither a type not a variable pointer. It's simply a decoration to structure types saying that this structure has a 'transparent memory layout' so any one of its member can be addressed by a pair of a pointer and an offset. The explicitly stated layout allows us to communicate with the device from the host, i.e., the CPU. For most of the cases we get such pointers from OpVariable declarations.

For a simplest example, try compiling the following two shaders with glslang targeting at SPIR-V 1.5:

// non-block.comp
#version 460 core
struct A { int i; };
void main() {
    A a;
    a.i = 0;
}

// block.comp
#version 460 core
layout(binding=0) buffer A { int i; } a;
void main() {
    a.i = 0;
}

And you will find out that there is no Block decoration on struct A in the first shader.

We certainly can record the pointer types with their storage classes attached, but it feels unnecessary to me because SPIR-Q doesn't actually care about what it points to. It's simply a pointer to somewhere. If its a known type; inline it. If the pointer's StorageClass is not PhysicalStorageBuffer, it is erroneous, but it should be intercepted by a preliminary validation pass rather than SPIR-Q. SPIR-Q is not a validator and it only reports fatal errors that fails it from the most fundamental reflection process.

[PENGUINLIONG]

Also SPIR-Q doesn't map every types SPIR-V provides, it only strictly reflect the types that might be interesting to the shader users.

[ktmattson-alion]

Point taken. I'm dragging myself through understanding the spec, so I still equivocate.

I've done some work to show what I mean, consider this arrangement:

layout(buffer_reference, scalar) buffer RefBuf1 {
    float i;
};

layout(buffer_reference, scalar) buffer RefBuf2 {
    float i;
    RefBuf1 ref;
};

layout(set=1,binding=0) buffer Bufs{
    RefBuf2 r;
} bufs;

The current library produces
Descriptor { name: Some("bufs"), desc_bind: (set=1, bind=0), desc_ty: StorageBuffer(ReadWrite), ty: Bufs { r: <pointer> { RefBuf2 { i: f32, ref: uint64_t } } }, nbind: 1 }
so you can tell that there is a ref inside the pointer, but not the type of the ref, because the struct resolution captured the forward pointer.
My changes give this:
Descriptor { name: Some("bufs"), desc_bind: (set=1, bind=0), desc_ty: StorageBuffer(ReadWrite), ty: Bufs { r: <pointer> { RefBuf2 { i: f32, ref: <pointer> { RefBuf1 { i: f32 } } } } }, nbind: 1 }
so that now it's clear that the ref member of RefBuf2 is a pointer to its precise datatype.

This makes sense to me because ForwardPointer types must have regular Pointer types attached to them before they are used, so you may as well propagate them.

[ktmattson-alion]

It's not perfect, since the linked-list case from e.g. https://github.com/KhronosGroup/GLSL/blob/master/extensions/ext/GLSL_EXT_buffer_reference.txt
causes infinite recursion. But I think the idea is clearer now anyway.

[PENGUINLIONG]

I think I get your point here. I had the same concern implementing this feature but in the end I decided not to do so because it adds an extra pass to transform all DeviceAddress to DevicePointer and I have to keep an extra piece of memory for the type mapping.

The decision was made based on the fact that our Types are referenced by Boxes. And it feels like a more serious cause of overallocation. So I'm gonna refactorize that first. But please feel free to PR the change about OpTypeForwardPointer later. :)

[ktmattson-alion]

Sounds good. If it's any help when thinking of that part, either for inspiration or "what not to do," I got what I wanted by changing the box to Rc<RefCell<Type>> in PointerType, and having forward pointers be registered as DevicePointer(DeviceAddress).

That way the real pointer can update the forward pointer when we find out what it is. On the other hand, it leaves the recursion problem for self-referential structs.