Add runtime array type #596
Conversation
| arr = in(reg) self, | ||
| index = in(reg) index, | ||
| } | ||
| loop {} |
There was a problem hiding this comment.
You should be able to replace the loop {} with options(noreturn) in the asm block.
https://doc.rust-lang.org/nightly/unstable-book/library-features/asm.html#options
https://doc.rust-lang.org/nightly/unstable-book/library-features/asm.html#options-1
There was a problem hiding this comment.
There is an open issue for supporting it: #570
There was a problem hiding this comment.
Ah, my bad. I should've seen that beforehand.
| arr = in(reg) self, | ||
| index = in(reg) index, | ||
| } | ||
| loop {} |
There was a problem hiding this comment.
Descriptor indexing also works for buffers, both sized and unsized as in #389. For that to work, there needs to be a separate type for DescriptorArray and RuntimeDescriptorArray because arrays and slices in storage buffer or uniform storage are inferred to be buffers that need to be wrapped in SpirvType::InterfaceBlock. It also makes it a little bit clearer what the type is for, someone was lil confused in chat on the weekend about it. I didn't know this was in progress, and it's something I need so I had started working on it myself about the same time you did. Kind of a shame to do duplicate work, but it's essentially the extension of this that allows for (possibly variably sized) buffers as well as opaque resources. See #597
| pub struct RuntimeArray<T> { | ||
| // spooky! this field does not exist, so if it's referenced in rust code, things will explode | ||
| _do_not_touch: u32, | ||
| _phantom: PhantomData<T>, | ||
| } |
There was a problem hiding this comment.
Calling this RuntimeArray is a bit confusing, someone was already in chat asking whether this was used for runtime data arrays. RuntimeDescriptorArray is a bit verbose, but it's also unambiguous, and for descriptor arrays of buffers this type needs to insert a block decorated struct around the buffer and have special indexing behaviour to handle that.
The u32 field is a bit strange, descriptor arrays, like descriptors, don't really have size/alignment per se. Since the compiler is handling this specially anyways, is it necessary?
There was a problem hiding this comment.
Since the compiler is handling this specially anyways, is it necessary?
You have to avoid making it a ZST, because those can be assumed to be data-less tokens, i.e. they have only "existence" but no memory/register representation. For example, (SomeZST, f32) is represented as just an f32, and so there is no way to recover any data the backend may want to keep in the ZST type.
The other option (instead of a dummy field) is a !Sized type, via extern { type Foo; }, which we should maybe move to at some point.
There was a problem hiding this comment.
Yeah, I'd love to move to extern { type Foo; }, but IIRC it requires a #[feature] and I'd like to keep those limited if at all possible (and AFAIK there's no other way to create an unsized yet no wide pointer type)
| asm! { | ||
| "%result = OpAccessChain _ {arr} {index}", | ||
| "OpReturnValue %result", | ||
| "%unused = OpLabel", | ||
| arr = in(reg) self, | ||
| index = in(reg) index, | ||
| } |
There was a problem hiding this comment.
This only works for the opaque resource types, extending this to support buffers requires a compiler implemented function.
| // The spirv type of it will be generated by querying the type of the first generic. | ||
| if let Some(elem_ty) = substs.types().next() { | ||
| let element = trans_type_impl(cx, span, cx.layout_of(elem_ty), false); | ||
| Ok(SpirvType::RuntimeArray { element }.def(span, cx)) |
There was a problem hiding this comment.
.def() produces an array stride decoration, which isn't necessary for opaque resource types (shader-playground GLSL example) and is explicitly forbidden for buffers.
| if is_pair { | ||
| self.tcx.sess.span_err( | ||
| hir_param.ty_span, | ||
| "uniform_constant buffers must use &RuntimeArray<T>, not &[T]", |
There was a problem hiding this comment.
uniform_constant buffers
I thought UniformConstant was only for non-buffers?
There was a problem hiding this comment.
An array of Image2d must use a direct OpTypeRuntimeArray not wrapped in a struct, i.e. &RuntimeArray<Image2d> instead of &[Image2d] (which is a large part of the point of this PR, supporting runtime arrays of images)
There was a problem hiding this comment.
Oh, but the word buffer might be incorrect here, idk what "an array of images" is called, it might not be a "buffer" so the error message might be able to be improved, but I think the code is correct.
| // is_pair implies is_unsized (but is_unsized doesn't imply is_pair, for RuntimeArray<T>) | ||
| assert!(!is_pair || is_unsized); |
There was a problem hiding this comment.
Can't this become really confusing if someone uses some type that just happens to be represented as a pair? I think you could replace if is_pair { below with if is_unsized_with_len { given:
let is_unsized_with_len = is_unsized && matches!(entry_arg_abi.mode, PassMode::Pair(..));
This allows runtime descriptor arrays, which aren't wrapped in a block struct, and have no length.