[spv-in] move all the image-related logic into a sub-module

src/front/spv/function.rs

@@ -1,7 +1,6 @@
-use crate::arena::Handle;
+use crate::arena::{Arena, Handle};
 
-use super::flow::*;
-use super::*;
+use super::{flow::*, Error, Instruction, LookupExpression, LookupHelper as _};
 
 pub type BlockId = u32;
 
@@ -50,13 +49,7 @@ pub enum Terminator {
 }
 
 impl<I: Iterator<Item = u32>> super::Parser<I> {
-    pub fn parse_function(
-        &mut self,
-        inst: Instruction,
-        module: &mut crate::Module,
-    ) -> Result<(), Error> {
-        self.switch(ModuleState::Function, inst.op)?;
-        inst.expect(5)?;
+    pub fn parse_function(&mut self, module: &mut crate::Module) -> Result<(), Error> {
         let result_type_id = self.next()?;
         let fun_id = self.next()?;
         let _fun_control = self.next()?;

src/front/spv/image.rs

@@ -0,0 +1,511 @@
+use crate::arena::{Arena, Handle};
+
+use super::{Error, LookupExpression, LookupHelper as _};
+
+#[derive(Clone, Debug)]
+pub(super) struct LookupSampledImage {
+    image: Handle<crate::Expression>,
+    sampler: Handle<crate::Expression>,
+}
+
+bitflags::bitflags! {
+    /// Flags describing sampling method.
+    pub struct SamplingFlags: u32 {
+        /// Regular sampling.
+        const REGULAR = 0x1;
+        /// Comparison sampling.
+        const COMPARISON = 0x2;
+    }
+}
+
+/// Return the texture coordinates separated from the array layer,
+/// and/or divided by the projection term.
+///
+/// The Proj sampling ops expect an extra coordinate for the W.
+/// The arrayed (can't be Proj!) images expect an extra coordinate for the layer.
+fn extract_image_coordinates(
+    image_dim: crate::ImageDimension,
+    image_arrayed: bool,
+    base: Handle<crate::Expression>,
+    coordinate_ty: Handle<crate::Type>,
+    type_arena: &Arena<crate::Type>,
+    expressions: &mut Arena<crate::Expression>,
+) -> (Handle<crate::Expression>, Option<Handle<crate::Expression>>) {
+    let (given_size, kind) = match type_arena[coordinate_ty].inner {
+        crate::TypeInner::Scalar { kind, .. } => (None, kind),
+        crate::TypeInner::Vector { size, kind, .. } => (Some(size), kind),
+        ref other => unreachable!("Unexpected texture coordinate {:?}", other),
+    };
+
+    let required_size = image_dim.required_coordinate_size();
+    let required_ty = required_size.map(|size| {
+        type_arena
+            .fetch_if(|ty| {
+                ty.inner
+                    == crate::TypeInner::Vector {
+                        size,
+                        kind,
+                        width: 4,
+                    }
+            })
+            .expect("Required coordinate type should have been set up by `parse_type_image`!")
+    });
+    let extra_expr = crate::Expression::AccessIndex {
+        base,
+        index: required_size.map_or(1, |size| size as u32),
+    };
+
+    if given_size == required_size {
+        // fast path, no complications
+        (base, None)
+    } else if image_arrayed {
+        // extra coordinate for the array index
+        let extracted = match required_size {
+            None => expressions.append(crate::Expression::AccessIndex { base, index: 0 }),
+            Some(size) => {
+                let mut components = Vec::with_capacity(size as usize);
+                for index in 0..size as u32 {
+                    let comp = expressions.append(crate::Expression::AccessIndex { base, index });
+                    components.push(comp);
+                }
+                expressions.append(crate::Expression::Compose {
+                    ty: required_ty.unwrap(),
+                    components,
+                })
+            }
+        };
+        let array_index_f32 = expressions.append(extra_expr);
+        let array_index = expressions.append(crate::Expression::As {
+            kind: crate::ScalarKind::Uint,
+            expr: array_index_f32,
+            convert: true,
+        });
+        (extracted, Some(array_index))
+    } else {
+        // extra coordinate for the projection W
+        let projection = expressions.append(extra_expr);
+        let divided = match required_size {
+            None => {
+                let temp = expressions.append(crate::Expression::AccessIndex { base, index: 0 });
+                expressions.append(crate::Expression::Binary {
+                    op: crate::BinaryOperator::Divide,
+                    left: temp,
+                    right: projection,
+                })
+            }
+            Some(size) => {
+                let mut components = Vec::with_capacity(size as usize);
+                for index in 0..size as u32 {
+                    let temp = expressions.append(crate::Expression::AccessIndex { base, index });
+                    let comp = expressions.append(crate::Expression::Binary {
+                        op: crate::BinaryOperator::Divide,
+                        left: temp,
+                        right: projection,
+                    });
+                    components.push(comp);
+                }
+                expressions.append(crate::Expression::Compose {
+                    ty: required_ty.unwrap(),
+                    components,
+                })
+            }
+        };
+        (divided, None)
+    }
+}
+
+pub(super) fn patch_comparison_type(
+    flags: SamplingFlags,
+    var: &mut crate::GlobalVariable,
+    arena: &mut Arena<crate::Type>,
+) -> bool {
+    if !flags.contains(SamplingFlags::COMPARISON) {
+        return true;
+    }
+    if flags == SamplingFlags::all() {
+        return false;
+    }
+
+    log::debug!("Flipping comparison for {:?}", var);
+    let original_ty = &arena[var.ty];
+    let ty_inner = match original_ty.inner {
+        crate::TypeInner::Image {
+            class: _,
+            dim,
+            arrayed,
+        } => crate::TypeInner::Image {
+            class: crate::ImageClass::Depth,
+            dim,
+            arrayed,
+        },
+        crate::TypeInner::Sampler { .. } => crate::TypeInner::Sampler { comparison: true },
+        ref other => unreachable!("Unexpected type for comparison mutation: {:?}", other),
+    };
+
+    let name = original_ty.name.clone();
+    var.ty = arena.append(crate::Type {
+        name,
+        inner: ty_inner,
+    });
+    true
+}
+
+impl<I: Iterator<Item = u32>> super::Parser<I> {
+    pub(super) fn parse_image_couple(&mut self) -> Result<(), Error> {
+        let _result_type_id = self.next()?;
+        let result_id = self.next()?;
+        let image_id = self.next()?;
+        let sampler_id = self.next()?;
+        let image_lexp = self.lookup_expression.lookup(image_id)?;
+        let sampler_lexp = self.lookup_expression.lookup(sampler_id)?;
+        self.lookup_sampled_image.insert(
+            result_id,
+            LookupSampledImage {
+                image: image_lexp.handle,
+                sampler: sampler_lexp.handle,
+            },
+        );
+        Ok(())
+    }
+
+    pub(super) fn parse_image_uncouple(&mut self) -> Result<(), Error> {
+        let result_type_id = self.next()?;
+        let result_id = self.next()?;
+        let sampled_image_id = self.next()?;
+        self.lookup_expression.insert(
+            result_id,
+            LookupExpression {
+                handle: self.lookup_sampled_image.lookup(sampled_image_id)?.image,
+                type_id: result_type_id,
+            },
+        );
+        Ok(())
+    }
+
+    pub(super) fn parse_image_fetch(
+        &mut self,
+        mut words_left: u16,
+        type_arena: &Arena<crate::Type>,
+        global_arena: &Arena<crate::GlobalVariable>,
+        expressions: &mut Arena<crate::Expression>,
+    ) -> Result<(), Error> {
+        let result_type_id = self.next()?;
+        let result_id = self.next()?;
+        let image_id = self.next()?;
+        let coordinate_id = self.next()?;
+
+        let mut index = None;
+        while words_left != 0 {
+            let image_ops = self.next()?;
+            words_left -= 1;
+            match spirv::ImageOperands::from_bits_truncate(image_ops) {
+                spirv::ImageOperands::LOD => {
+                    let lod_expr = self.next()?;
+                    let lod_handle = self.lookup_expression.lookup(lod_expr)?.handle;
+                    index = Some(lod_handle);
+                    words_left -= 1;
+                }
+                spirv::ImageOperands::SAMPLE => {
+                    let sample_expr = self.next()?;
+                    let sample_handle = self.lookup_expression.lookup(sample_expr)?.handle;
+                    index = Some(sample_handle);
+                    words_left -= 1;
+                }
+                other => {
+                    log::warn!("Skipping {:?}", other);
+                    for _ in 0..words_left {
+                        self.next()?;
+                    }
+                    break;
+                }
+            }
+        }
+
+        let image_lexp = self.lookup_expression.lookup(image_id)?;
+        let image_var_handle = expressions[image_lexp.handle].as_global_var()?;
+        let image_var = &global_arena[image_var_handle];
+
+        let coord_lexp = self.lookup_expression.lookup(coordinate_id)?;
+        let coord_type_handle = self.lookup_type.lookup(coord_lexp.type_id)?.handle;
+        let (coordinate, array_index) = match type_arena[image_var.ty].inner {
+            crate::TypeInner::Image {
+                dim,
+                arrayed,
+                class: _,
+            } => extract_image_coordinates(
+                dim,
+                arrayed,
+                coord_lexp.handle,
+                coord_type_handle,
+                type_arena,
+                expressions,
+            ),
+            _ => return Err(Error::InvalidSampleImage(image_var.ty)),
+        };
+
+        let expr = crate::Expression::ImageLoad {
+            image: image_lexp.handle,
+            coordinate,
+            array_index,
+            index,
+        };
+        self.lookup_expression.insert(
+            result_id,
+            LookupExpression {
+                handle: expressions.append(expr),
+                type_id: result_type_id,
+            },
+        );
+        Ok(())
+    }
+
+    pub(super) fn parse_image_sample(
+        &mut self,
+        mut words_left: u16,
+        type_arena: &Arena<crate::Type>,
+        global_arena: &Arena<crate::GlobalVariable>,
+        expressions: &mut Arena<crate::Expression>,
+    ) -> Result<(), Error> {
+        let result_type_id = self.next()?;
+        let result_id = self.next()?;
+        let sampled_image_id = self.next()?;
+        let coordinate_id = self.next()?;
+
+        let mut level = crate::SampleLevel::Auto;
+        while words_left != 0 {
+            let image_ops = self.next()?;
+            words_left -= 1;
+            match spirv::ImageOperands::from_bits_truncate(image_ops) {
+                spirv::ImageOperands::BIAS => {
+                    let bias_expr = self.next()?;
+                    let bias_handle = self.lookup_expression.lookup(bias_expr)?.handle;
+                    level = crate::SampleLevel::Bias(bias_handle);
+                    words_left -= 1;
+                }
+                spirv::ImageOperands::LOD => {
+                    let lod_expr = self.next()?;
+                    let lod_handle = self.lookup_expression.lookup(lod_expr)?.handle;
+                    level = crate::SampleLevel::Exact(lod_handle);
+                    words_left -= 1;
+                }
+                other => {
+                    log::warn!("Skipping {:?}", other);
+                    for _ in 0..words_left {
+                        self.next()?;
+                    }
+                    break;
+                }
+            }
+        }
+
+        let si_lexp = self.lookup_sampled_image.lookup(sampled_image_id)?;
+        let coord_lexp = self.lookup_expression.lookup(coordinate_id)?;
+        let coord_type_handle = self.lookup_type.lookup(coord_lexp.type_id)?.handle;
+
+        let image_var_handle = expressions[si_lexp.image].as_global_var()?;
+        let sampler_var_handle = expressions[si_lexp.sampler].as_global_var()?;
+        log::debug!(
+            "\t\t\tImage {:?} sampled with {:?}",
+            image_var_handle,
+            sampler_var_handle
+        );
+        if let Some(flags) = self.handle_sampling.get_mut(&image_var_handle) {
+            *flags |= SamplingFlags::REGULAR;
+        }
+        *self.handle_sampling.get_mut(&sampler_var_handle).unwrap() |= SamplingFlags::REGULAR;
+
+        let image_var = &global_arena[image_var_handle];
+        let (coordinate, array_index) = match type_arena[image_var.ty].inner {
+            crate::TypeInner::Image {
+                dim,
+                arrayed,
+                class: _,
+            } => extract_image_coordinates(
+                dim,
+                arrayed,
+                coord_lexp.handle,
+                coord_type_handle,
+                type_arena,
+                expressions,
+            ),
+            _ => return Err(Error::InvalidSampleImage(image_var.ty)),
+        };
+
+        let expr = crate::Expression::ImageSample {
+            image: si_lexp.image,
+            sampler: si_lexp.sampler,
+            coordinate,
+            array_index,
+            offset: None, //TODO
+            level,
+            depth_ref: None,
+        };
+        self.lookup_expression.insert(
+            result_id,
+            LookupExpression {
+                handle: expressions.append(expr),
+                type_id: result_type_id,
+            },
+        );
+        Ok(())
+    }
+
+    pub(super) fn parse_image_sample_dref(
+        &mut self,
+        mut words_left: u16,
+        type_arena: &Arena<crate::Type>,
+        global_arena: &Arena<crate::GlobalVariable>,
+        expressions: &mut Arena<crate::Expression>,
+    ) -> Result<(), Error> {
+        let result_type_id = self.next()?;
+        let result_id = self.next()?;
+        let sampled_image_id = self.next()?;
+        let coordinate_id = self.next()?;
+        let dref_id = self.next()?;
+
+        let mut level = crate::SampleLevel::Auto;
+        while words_left != 0 {
+            let image_ops = self.next()?;
+            words_left -= 1;
+            match spirv::ImageOperands::from_bits_truncate(image_ops) {
+                spirv::ImageOperands::BIAS => {
+                    let bias_expr = self.next()?;
+                    let bias_handle = self.lookup_expression.lookup(bias_expr)?.handle;
+                    level = crate::SampleLevel::Bias(bias_handle);
+                    words_left -= 1;
+                }
+                spirv::ImageOperands::LOD => {
+                    let lod_expr = self.next()?;
+                    let lod_handle = self.lookup_expression.lookup(lod_expr)?.handle;
+                    level = crate::SampleLevel::Exact(lod_handle);
+                    words_left -= 1;
+                }
+                other => {
+                    log::warn!("Skipping {:?}", other);
+                    for _ in 0..words_left {
+                        self.next()?;
+                    }
+                    break;
+                }
+            }
+        }
+
+        let si_lexp = self.lookup_sampled_image.lookup(sampled_image_id)?;
+        let coord_lexp = self.lookup_expression.lookup(coordinate_id)?;
+        let coord_type_handle = self.lookup_type.lookup(coord_lexp.type_id)?.handle;
+        let image_var_handle = expressions[si_lexp.image].as_global_var()?;
+        let sampler_var_handle = expressions[si_lexp.sampler].as_global_var()?;
+        log::debug!(
+            "\t\t\tImage {:?} sampled with comparison {:?}",
+            image_var_handle,
+            sampler_var_handle
+        );
+        if let Some(flags) = self.handle_sampling.get_mut(&image_var_handle) {
+            *flags |= SamplingFlags::COMPARISON;
+        }
+        *self.handle_sampling.get_mut(&sampler_var_handle).unwrap() |= SamplingFlags::COMPARISON;
+
+        let dref_lexp = self.lookup_expression.lookup(dref_id)?;
+        let dref_type_handle = self.lookup_type.lookup(dref_lexp.type_id)?.handle;
+        match type_arena[dref_type_handle].inner {
+            crate::TypeInner::Scalar {
+                kind: crate::ScalarKind::Float,
+                width: _,
+            } => (),
+            _ => return Err(Error::InvalidDepthReference(dref_type_handle)),
+        }
+
+        let image_var = &global_arena[image_var_handle];
+        let (coordinate, array_index) = match type_arena[image_var.ty].inner {
+            crate::TypeInner::Image {
+                dim,
+                arrayed,
+                class: _,
+            } => extract_image_coordinates(
+                dim,
+                arrayed,
+                coord_lexp.handle,
+                coord_type_handle,
+                type_arena,
+                expressions,
+            ),
+            _ => return Err(Error::InvalidSampleImage(image_var.ty)),
+        };
+
+        let expr = crate::Expression::ImageSample {
+            image: si_lexp.image,
+            sampler: si_lexp.sampler,
+            coordinate,
+            array_index,
+            offset: None, //TODO
+            level,
+            depth_ref: Some(dref_lexp.handle),
+        };
+        self.lookup_expression.insert(
+            result_id,
+            LookupExpression {
+                handle: expressions.append(expr),
+                type_id: result_type_id,
+            },
+        );
+        Ok(())
+    }
+
+    pub(super) fn parse_image_query_size(
+        &mut self,
+        at_level: bool,
+        expressions: &mut Arena<crate::Expression>,
+    ) -> Result<(), Error> {
+        let result_type_id = self.next()?;
+        let result_id = self.next()?;
+        let image_id = self.next()?;
+        let level = if at_level {
+            let level_id = self.next()?;
+            let level_lexp = self.lookup_expression.lookup(level_id)?;
+            Some(level_lexp.handle)
+        } else {
+            None
+        };
+
+        //TODO: handle arrays and cubes
+        let image_lexp = self.lookup_expression.lookup(image_id)?;
+
+        let expr = crate::Expression::ImageQuery {
+            image: image_lexp.handle,
+            query: crate::ImageQuery::Size { level },
+        };
+        self.lookup_expression.insert(
+            result_id,
+            LookupExpression {
+                handle: expressions.append(expr),
+                type_id: result_type_id,
+            },
+        );
+        Ok(())
+    }
+
+    pub(super) fn parse_image_query_other(
+        &mut self,
+        query: crate::ImageQuery,
+        expressions: &mut Arena<crate::Expression>,
+    ) -> Result<(), Error> {
+        let result_type_id = self.next()?;
+        let result_id = self.next()?;
+        let image_id = self.next()?;
+
+        let image_lexp = self.lookup_expression.lookup(image_id)?.clone();
+
+        let expr = crate::Expression::ImageQuery {
+            image: image_lexp.handle,
+            query,
+        };
+        self.lookup_expression.insert(
+            result_id,
+            LookupExpression {
+                handle: expressions.append(expr),
+                type_id: result_type_id,
+            },
+        );
+        Ok(())
+    }
+}

src/front/spv/mod.rs

@@ -15,6 +15,7 @@ mod convert;
 mod error;
 mod flow;
 mod function;
+mod image;
 #[cfg(all(test, feature = "serialize"))]
 mod rosetta;
 
@@ -55,12 +56,10 @@ impl Instruction {
         }
     }
 
-    fn expect_at_least(self, count: u16) -> Result<(), Error> {
-        if self.wc >= count {
-            Ok(())
-        } else {
-            Err(Error::InvalidOperandCount(self.op, self.wc))
-        }
+    fn expect_at_least(self, count: u16) -> Result<u16, Error> {
+        self.wc
+            .checked_sub(count)
+            .ok_or(Error::InvalidOperandCount(self.op, self.wc))
     }
 }
 
@@ -163,102 +162,6 @@ impl crate::ImageDimension {
     }
 }
 
-/// Return the texture coordinates separated from the array layer,
-/// and/or divided by the projection term.
-///
-/// The Proj sampling ops expect an extra coordinate for the W.
-/// The arrayed (can't be Proj!) images expect an extra coordinate for the layer.
-fn extract_image_coordinates(
-    image_dim: crate::ImageDimension,
-    image_arrayed: bool,
-    base: Handle<crate::Expression>,
-    coordinate_ty: Handle<crate::Type>,
-    type_arena: &Arena<crate::Type>,
-    expressions: &mut Arena<crate::Expression>,
-) -> (Handle<crate::Expression>, Option<Handle<crate::Expression>>) {
-    let (given_size, kind) = match type_arena[coordinate_ty].inner {
-        crate::TypeInner::Scalar { kind, .. } => (None, kind),
-        crate::TypeInner::Vector { size, kind, .. } => (Some(size), kind),
-        ref other => unreachable!("Unexpected texture coordinate {:?}", other),
-    };
-
-    let required_size = image_dim.required_coordinate_size();
-    let required_ty = required_size.map(|size| {
-        type_arena
-            .fetch_if(|ty| {
-                ty.inner
-                    == crate::TypeInner::Vector {
-                        size,
-                        kind,
-                        width: 4,
-                    }
-            })
-            .expect("Required coordinate type should have been set up by `parse_type_image`!")
-    });
-    let extra_expr = crate::Expression::AccessIndex {
-        base,
-        index: required_size.map_or(1, |size| size as u32),
-    };
-
-    if given_size == required_size {
-        // fast path, no complications
-        (base, None)
-    } else if image_arrayed {
-        // extra coordinate for the array index
-        let extracted = match required_size {
-            None => expressions.append(crate::Expression::AccessIndex { base, index: 0 }),
-            Some(size) => {
-                let mut components = Vec::with_capacity(size as usize);
-                for index in 0..size as u32 {
-                    let comp = expressions.append(crate::Expression::AccessIndex { base, index });
-                    components.push(comp);
-                }
-                expressions.append(crate::Expression::Compose {
-                    ty: required_ty.unwrap(),
-                    components,
-                })
-            }
-        };
-        let array_index_f32 = expressions.append(extra_expr);
-        let array_index = expressions.append(crate::Expression::As {
-            kind: crate::ScalarKind::Uint,
-            expr: array_index_f32,
-            convert: true,
-        });
-        (extracted, Some(array_index))
-    } else {
-        // extra coordinate for the projection W
-        let projection = expressions.append(extra_expr);
-        let divided = match required_size {
-            None => {
-                let temp = expressions.append(crate::Expression::AccessIndex { base, index: 0 });
-                expressions.append(crate::Expression::Binary {
-                    op: crate::BinaryOperator::Divide,
-                    left: temp,
-                    right: projection,
-                })
-            }
-            Some(size) => {
-                let mut components = Vec::with_capacity(size as usize);
-                for index in 0..size as u32 {
-                    let temp = expressions.append(crate::Expression::AccessIndex { base, index });
-                    let comp = expressions.append(crate::Expression::Binary {
-                        op: crate::BinaryOperator::Divide,
-                        left: temp,
-                        right: projection,
-                    });
-                    components.push(comp);
-                }
-                expressions.append(crate::Expression::Compose {
-                    ty: required_ty.unwrap(),
-                    components,
-                })
-            }
-        };
-        (divided, None)
-    }
-}
-
 type MemberIndex = u32;
 
 #[derive(Clone, Debug, Default, PartialEq)]
@@ -313,7 +216,7 @@ impl Decoration {
                 desc_set: None,
                 desc_index: None,
                 ..
-            } => match convert::map_builtin(built_in, is_output) {
+            } => match map_builtin(built_in, is_output) {
                 Ok(built_in) => Some(crate::Binding::BuiltIn(built_in)),
                 Err(e) => {
                     log::warn!("{:?}", e);
@@ -339,16 +242,6 @@ impl Decoration {
     }
 }
 
-bitflags::bitflags! {
-    /// Flags describing sampling method.
-    pub struct SamplingFlags: u32 {
-        /// Regular sampling.
-        const REGULAR = 0x1;
-        /// Comparison sampling.
-        const COMPARISON = 0x2;
-    }
-}
-
 #[derive(Debug)]
 struct LookupFunctionType {
     parameter_type_ids: Vec<spirv::Word>,
@@ -389,12 +282,6 @@ struct LookupExpression {
     type_id: spirv::Word,
 }
 
-#[derive(Clone, Debug)]
-struct LookupSampledImage {
-    image: Handle<crate::Expression>,
-    sampler: Handle<crate::Expression>,
-}
-
 #[derive(Clone, Debug)]
 pub struct Assignment {
     to: Handle<crate::Expression>,
@@ -414,7 +301,7 @@ pub struct Parser<I> {
     future_decor: FastHashMap<spirv::Word, Decoration>,
     future_member_decor: FastHashMap<(spirv::Word, MemberIndex), Decoration>,
     lookup_member_type_id: FastHashMap<(Handle<crate::Type>, MemberIndex), spirv::Word>,
-    handle_sampling: FastHashMap<Handle<crate::GlobalVariable>, SamplingFlags>,
+    handle_sampling: FastHashMap<Handle<crate::GlobalVariable>, image::SamplingFlags>,
     lookup_type: FastHashMap<spirv::Word, LookupType>,
     lookup_void_type: Option<spirv::Word>,
     lookup_storage_buffer_types: FastHashSet<Handle<crate::Type>>,
@@ -422,7 +309,7 @@ pub struct Parser<I> {
     lookup_constant: FastHashMap<spirv::Word, LookupConstant>,
     lookup_variable: FastHashMap<spirv::Word, LookupVariable>,
     lookup_expression: FastHashMap<spirv::Word, LookupExpression>,
-    lookup_sampled_image: FastHashMap<spirv::Word, LookupSampledImage>,
+    lookup_sampled_image: FastHashMap<spirv::Word, image::LookupSampledImage>,
     lookup_function_type: FastHashMap<spirv::Word, LookupFunctionType>,
     lookup_function: FastHashMap<spirv::Word, Handle<crate::Function>>,
     lookup_entry_point: FastHashMap<spirv::Word, EntryPoint>,
@@ -1104,378 +991,47 @@ impl<I: Iterator<Item = u32>> Parser<I> {
                 // Sampling
                 Op::Image => {
                     inst.expect(4)?;
-                    let result_type_id = self.next()?;
-                    let result_id = self.next()?;
-                    let sampled_image_id = self.next()?;
-                    self.lookup_expression.insert(
-                        result_id,
-                        LookupExpression {
-                            handle: self.lookup_sampled_image.lookup(sampled_image_id)?.image,
-                            type_id: result_type_id,
-                        },
-                    );
+                    self.parse_image_uncouple()?;
                 }
                 Op::SampledImage => {
                     inst.expect(5)?;
-                    let _result_type_id = self.next()?;
-                    let result_id = self.next()?;
-                    let image_id = self.next()?;
-                    let sampler_id = self.next()?;
-                    let image_lexp = self.lookup_expression.lookup(image_id)?;
-                    let sampler_lexp = self.lookup_expression.lookup(sampler_id)?;
-                    //TODO: compare the result type
-                    self.lookup_sampled_image.insert(
-                        result_id,
-                        LookupSampledImage {
-                            image: image_lexp.handle,
-                            sampler: sampler_lexp.handle,
-                        },
-                    );
+                    self.parse_image_couple()?;
                 }
                 Op::ImageFetch => {
-                    inst.expect_at_least(5)?;
-                    let result_type_id = self.next()?;
-                    let result_id = self.next()?;
-                    let image_id = self.next()?;
-                    let coordinate_id = self.next()?;
-
-                    let mut index = None;
-                    let mut base_wc = 5;
-                    while base_wc < inst.wc {
-                        let image_ops = self.next()?;
-                        base_wc += 1;
-                        match spirv::ImageOperands::from_bits_truncate(image_ops) {
-                            spirv::ImageOperands::LOD => {
-                                let lod_expr = self.next()?;
-                                let lod_handle = self.lookup_expression.lookup(lod_expr)?.handle;
-                                index = Some(lod_handle);
-                                base_wc += 1;
-                            }
-                            spirv::ImageOperands::SAMPLE => {
-                                let sample_expr = self.next()?;
-                                let sample_handle =
-                                    self.lookup_expression.lookup(sample_expr)?.handle;
-                                index = Some(sample_handle);
-                                base_wc += 1;
-                            }
-                            other => {
-                                log::warn!("Skipping {:?}", other);
-                                for _ in base_wc..inst.wc {
-                                    self.next()?;
-                                }
-                                break;
-                            }
-                        }
-                    }
-
-                    let image_lexp = self.lookup_expression.lookup(image_id)?;
-                    let image_var_handle = expressions[image_lexp.handle].as_global_var()?;
-                    let image_var = &global_arena[image_var_handle];
-
-                    let coord_lexp = self.lookup_expression.lookup(coordinate_id)?;
-                    let coord_type_handle = self.lookup_type.lookup(coord_lexp.type_id)?.handle;
-                    let (coordinate, array_index) = match type_arena[image_var.ty].inner {
-                        crate::TypeInner::Image {
-                            dim,
-                            arrayed,
-                            class: _,
-                        } => extract_image_coordinates(
-                            dim,
-                            arrayed,
-                            coord_lexp.handle,
-                            coord_type_handle,
-                            type_arena,
-                            expressions,
-                        ),
-                        _ => return Err(Error::InvalidSampleImage(image_var.ty)),
-                    };
-
-                    let expr = crate::Expression::ImageLoad {
-                        image: image_lexp.handle,
-                        coordinate,
-                        array_index,
-                        index,
-                    };
-                    self.lookup_expression.insert(
-                        result_id,
-                        LookupExpression {
-                            handle: expressions.append(expr),
-                            type_id: result_type_id,
-                        },
-                    );
+                    let extra = inst.expect_at_least(5)?;
+                    self.parse_image_fetch(extra, type_arena, global_arena, expressions)?;
                 }
                 Op::ImageSampleImplicitLod
                 | Op::ImageSampleExplicitLod
                 | Op::ImageSampleProjImplicitLod
                 | Op::ImageSampleProjExplicitLod => {
-                    let mut base_wc = 5;
-                    inst.expect_at_least(base_wc)?;
-                    let result_type_id = self.next()?;
-                    let result_id = self.next()?;
-                    let sampled_image_id = self.next()?;
-                    let coordinate_id = self.next()?;
-
-                    let mut level = crate::SampleLevel::Auto;
-                    while base_wc < inst.wc {
-                        let image_ops = self.next()?;
-                        base_wc += 1;
-                        match spirv::ImageOperands::from_bits_truncate(image_ops) {
-                            spirv::ImageOperands::BIAS => {
-                                let bias_expr = self.next()?;
-                                let bias_handle = self.lookup_expression.lookup(bias_expr)?.handle;
-                                level = crate::SampleLevel::Bias(bias_handle);
-                                base_wc += 1;
-                            }
-                            spirv::ImageOperands::LOD => {
-                                let lod_expr = self.next()?;
-                                let lod_handle = self.lookup_expression.lookup(lod_expr)?.handle;
-                                level = crate::SampleLevel::Exact(lod_handle);
-                                base_wc += 1;
-                            }
-                            other => {
-                                log::warn!("Skipping {:?}", other);
-                                for _ in base_wc..inst.wc {
-                                    self.next()?;
-                                }
-                                break;
-                            }
-                        }
-                    }
-
-                    let si_lexp = self.lookup_sampled_image.lookup(sampled_image_id)?;
-                    let coord_lexp = self.lookup_expression.lookup(coordinate_id)?;
-                    let coord_type_handle = self.lookup_type.lookup(coord_lexp.type_id)?.handle;
-
-                    let image_var_handle = expressions[si_lexp.image].as_global_var()?;
-                    let sampler_var_handle = expressions[si_lexp.sampler].as_global_var()?;
-                    log::debug!(
-                        "\t\t\tImage {:?} sampled with {:?}",
-                        image_var_handle,
-                        sampler_var_handle
-                    );
-                    if let Some(flags) = self.handle_sampling.get_mut(&image_var_handle) {
-                        *flags |= SamplingFlags::REGULAR;
-                    }
-                    *self.handle_sampling.get_mut(&sampler_var_handle).unwrap() |=
-                        SamplingFlags::REGULAR;
-
-                    let image_var = &global_arena[image_var_handle];
-                    let (coordinate, array_index) = match type_arena[image_var.ty].inner {
-                        crate::TypeInner::Image {
-                            dim,
-                            arrayed,
-                            class: _,
-                        } => extract_image_coordinates(
-                            dim,
-                            arrayed,
-                            coord_lexp.handle,
-                            coord_type_handle,
-                            type_arena,
-                            expressions,
-                        ),
-                        _ => return Err(Error::InvalidSampleImage(image_var.ty)),
-                    };
-
-                    let expr = crate::Expression::ImageSample {
-                        image: si_lexp.image,
-                        sampler: si_lexp.sampler,
-                        coordinate,
-                        array_index,
-                        offset: None, //TODO
-                        level,
-                        depth_ref: None,
-                    };
-                    self.lookup_expression.insert(
-                        result_id,
-                        LookupExpression {
-                            handle: expressions.append(expr),
-                            type_id: result_type_id,
-                        },
-                    );
+                    let extra = inst.expect_at_least(5)?;
+                    self.parse_image_sample(extra, type_arena, global_arena, expressions)?;
                 }
                 Op::ImageSampleDrefImplicitLod
                 | Op::ImageSampleDrefExplicitLod
                 | Op::ImageSampleProjDrefImplicitLod
                 | Op::ImageSampleProjDrefExplicitLod => {
-                    let mut base_wc = 6;
-                    inst.expect_at_least(base_wc)?;
-                    let result_type_id = self.next()?;
-                    let result_id = self.next()?;
-                    let sampled_image_id = self.next()?;
-                    let coordinate_id = self.next()?;
-                    let dref_id = self.next()?;
-
-                    let mut level = crate::SampleLevel::Auto;
-                    while base_wc < inst.wc {
-                        let image_ops = self.next()?;
-                        base_wc += 1;
-                        match spirv::ImageOperands::from_bits_truncate(image_ops) {
-                            spirv::ImageOperands::BIAS => {
-                                let bias_expr = self.next()?;
-                                let bias_handle = self.lookup_expression.lookup(bias_expr)?.handle;
-                                level = crate::SampleLevel::Bias(bias_handle);
-                                base_wc += 1;
-                            }
-                            spirv::ImageOperands::LOD => {
-                                let lod_expr = self.next()?;
-                                let lod_handle = self.lookup_expression.lookup(lod_expr)?.handle;
-                                level = crate::SampleLevel::Exact(lod_handle);
-                                base_wc += 1;
-                            }
-                            other => {
-                                log::warn!("Skipping {:?}", other);
-                                for _ in base_wc..inst.wc {
-                                    self.next()?;
-                                }
-                                break;
-                            }
-                        }
-                    }
-
-                    let si_lexp = self.lookup_sampled_image.lookup(sampled_image_id)?;
-                    let coord_lexp = self.lookup_expression.lookup(coordinate_id)?;
-                    let coord_type_handle = self.lookup_type.lookup(coord_lexp.type_id)?.handle;
-                    let image_var_handle = expressions[si_lexp.image].as_global_var()?;
-                    let sampler_var_handle = expressions[si_lexp.sampler].as_global_var()?;
-                    log::debug!(
-                        "\t\t\tImage {:?} sampled with comparison {:?}",
-                        image_var_handle,
-                        sampler_var_handle
-                    );
-                    if let Some(flags) = self.handle_sampling.get_mut(&image_var_handle) {
-                        *flags |= SamplingFlags::COMPARISON;
-                    }
-                    *self.handle_sampling.get_mut(&sampler_var_handle).unwrap() |=
-                        SamplingFlags::COMPARISON;
-
-                    let dref_lexp = self.lookup_expression.lookup(dref_id)?;
-                    let dref_type_handle = self.lookup_type.lookup(dref_lexp.type_id)?.handle;
-                    match type_arena[dref_type_handle].inner {
-                        crate::TypeInner::Scalar {
-                            kind: crate::ScalarKind::Float,
-                            width: _,
-                        } => (),
-                        _ => return Err(Error::InvalidDepthReference(dref_type_handle)),
-                    }
-
-                    let image_var = &global_arena[image_var_handle];
-                    let (coordinate, array_index) = match type_arena[image_var.ty].inner {
-                        crate::TypeInner::Image {
-                            dim,
-                            arrayed,
-                            class: _,
-                        } => extract_image_coordinates(
-                            dim,
-                            arrayed,
-                            coord_lexp.handle,
-                            coord_type_handle,
-                            type_arena,
-                            expressions,
-                        ),
-                        _ => return Err(Error::InvalidSampleImage(image_var.ty)),
-                    };
-
-                    let expr = crate::Expression::ImageSample {
-                        image: si_lexp.image,
-                        sampler: si_lexp.sampler,
-                        coordinate,
-                        array_index,
-                        offset: None, //TODO
-                        level,
-                        depth_ref: Some(dref_lexp.handle),
-                    };
-                    self.lookup_expression.insert(
-                        result_id,
-                        LookupExpression {
-                            handle: expressions.append(expr),
-                            type_id: result_type_id,
-                        },
-                    );
+                    let extra = inst.expect_at_least(6)?;
+                    self.parse_image_sample_dref(extra, type_arena, global_arena, expressions)?;
                 }
                 Op::ImageQuerySize => {
                     inst.expect(4)?;
-                    let result_type_id = self.next()?;
-                    let result_id = self.next()?;
-                    let image_id = self.next()?;
-
-                    //TODO: handle arrays and cubes
-                    let image_lexp = self.lookup_expression.lookup(image_id)?.clone();
-
-                    let expr = crate::Expression::ImageQuery {
-                        image: image_lexp.handle,
-                        query: crate::ImageQuery::Size { level: None },
-                    };
-                    self.lookup_expression.insert(
-                        result_id,
-                        LookupExpression {
-                            handle: expressions.append(expr),
-                            type_id: result_type_id,
-                        },
-                    );
+                    self.parse_image_query_size(false, expressions)?;
                 }
                 Op::ImageQuerySizeLod => {
                     inst.expect(5)?;
-                    let result_type_id = self.next()?;
-                    let result_id = self.next()?;
-                    let image_id = self.next()?;
-                    let level_id = self.next()?;
-
-                    //TODO: handle arrays and cubes
-                    let image_lexp = self.lookup_expression.lookup(image_id)?.clone();
-                    let level_lexp = self.lookup_expression.lookup(level_id)?.clone();
-
-                    let expr = crate::Expression::ImageQuery {
-                        image: image_lexp.handle,
-                        query: crate::ImageQuery::Size {
-                            level: Some(level_lexp.handle),
-                        },
-                    };
-                    self.lookup_expression.insert(
-                        result_id,
-                        LookupExpression {
-                            handle: expressions.append(expr),
-                            type_id: result_type_id,
-                        },
-                    );
+                    self.parse_image_query_size(true, expressions)?;
                 }
                 Op::ImageQueryLevels => {
-                    let result_type_id = self.next()?;
-                    let result_id = self.next()?;
-                    let image_id = self.next()?;
-
-                    let image_lexp = self.lookup_expression.lookup(image_id)?.clone();
-
-                    let expr = crate::Expression::ImageQuery {
-                        image: image_lexp.handle,
-                        query: crate::ImageQuery::NumLevels,
-                    };
-                    self.lookup_expression.insert(
-                        result_id,
-                        LookupExpression {
-                            handle: expressions.append(expr),
-                            type_id: result_type_id,
-                        },
-                    );
+                    inst.expect(4)?;
+                    self.parse_image_query_other(crate::ImageQuery::NumLevels, expressions)?;
                 }
                 Op::ImageQuerySamples => {
-                    let result_type_id = self.next()?;
-                    let result_id = self.next()?;
-                    let image_id = self.next()?;
-
-                    let image_lexp = self.lookup_expression.lookup(image_id)?.clone();
-
-                    let expr = crate::Expression::ImageQuery {
-                        image: image_lexp.handle,
-                        query: crate::ImageQuery::NumSamples,
-                    };
-                    self.lookup_expression.insert(
-                        result_id,
-                        LookupExpression {
-                            handle: expressions.append(expr),
-                            type_id: result_type_id,
-                        },
-                    );
+                    inst.expect(4)?;
+                    self.parse_image_query_other(crate::ImageQuery::NumSamples, expressions)?;
                 }
+                // other ops
                 Op::Select => {
                     inst.expect(6)?;
                     let result_type_id = self.next()?;
@@ -1995,40 +1551,24 @@ impl<I: Iterator<Item = u32>> Parser<I> {
                 Op::ConstantComposite => self.parse_composite_constant(inst, &mut module),
                 Op::ConstantNull | Op::Undef => self.parse_null_constant(inst, &mut module),
                 Op::Variable => self.parse_global_variable(inst, &mut module),
-                Op::Function => self.parse_function(inst, &mut module),
+                Op::Function => {
+                    self.switch(ModuleState::Function, inst.op)?;
+                    inst.expect(5)?;
+                    self.parse_function(&mut module)
+                }
                 _ => Err(Error::UnsupportedInstruction(self.state, inst.op)), //TODO
             }?;
         }
 
         // Check all the images and samplers to have consistent comparison property.
         for (handle, flags) in self.handle_sampling.drain() {
-            if !flags.contains(SamplingFlags::COMPARISON) {
-                continue;
-            }
-            if flags == SamplingFlags::all() {
+            if !image::patch_comparison_type(
+                flags,
+                module.global_variables.get_mut(handle),
+                &mut module.types,
+            ) {
                 return Err(Error::InconsistentComparisonSampling(handle));
             }
-            log::debug!("Flipping comparison for {:?}", handle);
-            let var = module.global_variables.get_mut(handle);
-            let original_ty = &module.types[var.ty];
-            let ty_inner = match original_ty.inner {
-                crate::TypeInner::Image {
-                    class: _,
-                    dim,
-                    arrayed,
-                } => crate::TypeInner::Image {
-                    class: crate::ImageClass::Depth,
-                    dim,
-                    arrayed,
-                },
-                crate::TypeInner::Sampler { .. } => crate::TypeInner::Sampler { comparison: true },
-                ref other => unreachable!("Unexpected type for comparison mutation: {:?}", other),
-            };
-            let name = original_ty.name.clone();
-            var.ty = module.types.append(crate::Type {
-                name,
-                inner: ty_inner,
-            });
         }
 
         for (_, func) in module.functions.iter_mut() {
@@ -2971,7 +2511,8 @@ impl<I: Iterator<Item = u32>> Parser<I> {
             .can_comparison_sample()
         {
             log::debug!("\t\ttracking {:?} for sampling properties", handle);
-            self.handle_sampling.insert(handle, SamplingFlags::empty());
+            self.handle_sampling
+                .insert(handle, image::SamplingFlags::empty());
         }
         Ok(())
     }