Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a function to allow combining images and samplers into SampledImages (long term) #774

Closed
expenses opened this issue Oct 25, 2021 · 3 comments
Labels
t: enhancement A new feature or improvement to an existing one.

Comments

@expenses
Copy link
Contributor

It should be possible to implement something like this:

let sampled_image = image.combine(sampler);

as mentioned here: #320

This would be nice as we could move all the sampling functions onto SampledImage instead of duplicating them like at present.

Something like this would be great:

pub fn combine(&self, sampler: Sampler) -> SampledImage<Self> {
    unsafe {
        asm! {
            "%typeSampledImage = OpTypeSampledImage typeof*{this}",
            "%image = OpLoad _ {this}",
            "%sampler = OpLoad _ {sampler}",
            "%sampledImage = OpSampledImage %typeSampledImage %image %sampler",
            "OpReturnValue %sampledImage",
            this = in(reg) self,
            sampler = in(reg) &sampler,
            options(noreturn),
        }
    }
}

Except that SPIR-V disallows returning returning OpSampledImages from functions:

error: error:0:0 - Result <id> from OpSampledImage instruction must not appear as operand for OpReturnValue, since it is not specificed as taking an OpTypeSampledImage. Found result <id> '78[%78]' as an operand of <id> '0[%0]'.

if we add #[inline] to the function then we get an error about not being able to OpStore an OpSampledImage:

error: error:0:0 - Result <id> from OpSampledImage instruction must not appear as operand for OpStore, since it is not specificed as taking an OpTypeSampledImage. Found result <id> '84[%84]' as an operand of <id> '0[%0]'.

Therefore, in order to implement this we'd need a legalisation pass that removes all the (redundant) OpStores of OpSampledImages.

There might be other problems as well but I do believe that would be the way to go long term.

@expenses expenses added the t: enhancement A new feature or improvement to an existing one. label Oct 25, 2021
@khyperia
Copy link
Contributor

Unfortunately there are other problems that make this impossible to do 😢. For example, this bit of spec:

All OpSampledImage instructions must be in the same block in which their Result <id> are consumed. Result <id> from OpSampledImage instructions must not appear as operands to OpPhi instructions or OpSelect instructions, or any instructions other than the image lookup and query instructions specified to take an operand whose type is OpTypeSampledImage.

(an unsaid bit of the second part is "no passing to functions, either")

This is ridiculously restrictive, and completely impractical to expose in any public API - the fact that a call must be in the same basic block as its consumption is unintuitive, difficult to provide good error messages for, and I believe against what I think the intent of what the OpSampledImage instruction was created for: as a compiler codegenning tool to always be placed immediately before a sampling instruction, to not have to duplicate all sampling instructions to take either a combined or separate sampler. Maybe we can get the spec to change here to make OpSampledImage be an actually useful instruction to expose to users, but as it is right now, I don't think exposing it is the right way to go, sadly.

@expenses
Copy link
Contributor Author

Ah right, I assumed that the same restrictions would apply to SampledImages passed into shaders via descriptor sets, but that's not the case as the don't use OpSampledImage (the constructor function).

I think I'll still look into this to get a better view of what possible. IMO the 'no passing to functions' restriction is okay, given that functions are inlined via spirv-opt anyway. Requiring that it's consumed in the same block is presumably a problem for sampling in loops though.

@expenses
Copy link
Contributor Author

I wrote out the mentioned legalisation pass to do a little test: main...expenses:image-combine-test

Something like

let combined = unsafe { image2d_array.combine(*sampler) };

let r5: glam::Vec4 = unsafe { combined.sample(v3) };
let r6: glam::Vec4 = unsafe { combined.sample(v3 + glam::Vec3::splat(0.01)) };

works fine but

let mut sum = glam::Vec4::ZERO;

for _ in 0 .. 3 {
    let sample: glam::Vec4 = unsafe { combined.sample(v3) };
    sum += sample;
}

results in:

error: error:0:0 - All OpSampledImage instructions must be in the same block in which
 their Result <id> are consumed. OpSampledImage Result Type <id> '102[%102]' has a co
nsumer in a different basic block. The consumer instruction <id> is '188[%188]'.

Given that the error message isn't great and that we can't just unroll every loop that an OpSampledImage is sampled in, I definitely agree that we shouldn't expose the combine function, at least not with out some kind of 'you are on your own if you use this' comment.

I think it would be okay to use the combine function internally for Image::sample etc though, depending on how okay you are with having that legalisation pass and with SampledImage::sample etc. being #[inline].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: enhancement A new feature or improvement to an existing one.
Projects
None yet
Development

No branches or pull requests

2 participants