OpLoad wrongly classified as a shader access for images

[jrprice]

It looks like VVL treats OpLoad of an image handle as an access to the underlying texture storage, which is leading to false positives (e.g. SYNC-HAZARD-WRITE-AFTER-READ).

For example, a shader that only queries the dimensions of an image does not actually perform any accesses to that image. The Vulkan spec says:

OpImageQuerySize [...] return properties of the image descriptor that would be accessed. The image itself is not accessed.

I created this unit test for sync_val_positive.cpp which I believe captures the issue:

TEST_F(PositiveSyncVal, WriteAfterImageUsedInShaderWithoutAccess) {
    TEST_DESCRIPTION("Test that image queries are not considered to be accesses");
    RETURN_IF_SKIP(InitSyncValFramework());
    RETURN_IF_SKIP(InitState());

    vkt::Buffer copy_source(*m_device, 32 * 32 * 4, VK_BUFFER_USAGE_TRANSFER_SRC_BIT | VK_BUFFER_USAGE_STORAGE_BUFFER_BIT);

    vkt::Image image(*m_device, 32, 32, 1, VK_FORMAT_R8G8B8A8_UNORM, VK_IMAGE_USAGE_STORAGE_BIT | VK_IMAGE_USAGE_TRANSFER_DST_BIT);
    image.SetLayout(VK_IMAGE_LAYOUT_GENERAL);
    vkt::ImageView view = image.CreateView();

    OneOffDescriptorSet descriptor_set(m_device, {
                                                     {0, VK_DESCRIPTOR_TYPE_STORAGE_IMAGE, 1, VK_SHADER_STAGE_COMPUTE_BIT, nullptr},
                                                 });
    descriptor_set.WriteDescriptorImageInfo(0, view, VK_NULL_HANDLE, VK_DESCRIPTOR_TYPE_STORAGE_IMAGE, VK_IMAGE_LAYOUT_GENERAL);
    descriptor_set.UpdateDescriptorSets();

    const char* cs_source = R"glsl(
        #version 450
        layout(set = 0, binding = 0, rgba8) writeonly uniform image2D image;
        void main(){
            uvec2 size = imageSize(image);
        }
    )glsl";
    CreateComputePipelineHelper pipe(*this);
    pipe.cs_ = std::make_unique<VkShaderObj>(this, cs_source, VK_SHADER_STAGE_COMPUTE_BIT);
    pipe.pipeline_layout_ = vkt::PipelineLayout(*m_device, {&descriptor_set.layout_});
    pipe.CreateComputePipeline();

    VkBufferImageCopy region{};
    region.imageSubresource = {VK_IMAGE_ASPECT_COLOR_BIT, 0, 0, 1};
    region.imageExtent = {32, 32, 1};

    m_commandBuffer->begin();
    vk::CmdBindPipeline(*m_commandBuffer, VK_PIPELINE_BIND_POINT_COMPUTE, pipe.Handle());
    vk::CmdBindDescriptorSets(*m_commandBuffer, VK_PIPELINE_BIND_POINT_COMPUTE, pipe.pipeline_layout_, 0, 1, &descriptor_set.set_,
                              0, nullptr);
    vk::CmdDispatch(*m_commandBuffer, 1, 1, 1);
    vk::CmdCopyBufferToImage(*m_commandBuffer, copy_source, image, VK_IMAGE_LAYOUT_GENERAL, 1, &region);

    m_commandBuffer->end();
}

The test fails with the following error:

Validation Error: [ SYNC-HAZARD-WRITE-AFTER-READ ] Object 0: handle = 0x55fbabac7180, type = VK_OBJECT_TYPE_COMMAND_BUFFER; Object 1: handle = 0xf443490000000006, type = VK_OBJECT_TYPE_IMAGE; | MessageID = 0x376bc9df | vkCmdCopyBufferToImage():  Hazard WRITE_AFTER_READ for dstImage VkImage 0xf443490000000006[], region 0. Access info (usage: SYNC_COPY_TRANSFER_WRITE, prior_usage: SYNC_COMPUTE_SHADER_SHADER_STORAGE_READ, read_barriers: VkPipelineStageFlags2(0), command: vkCmdDispatch, seq_no: 1, reset_no: 1).

The error indicates that the usage includes SYNC_COMPUTE_SHADER_SHADER_STORAGE_READ, despite the fact that no accesses are performed by the shader. I believe the issue is specifically tied to the OpLoad instruction which loads the image handle, as I'm seeing these false positives with shaders that only do that.

[artem-lunarg]

@jrprice thanks for the test.

@spencer-lunarg I checked this test before the writeonly change and after that, and the error existed in both cases with this difference:

Before writeonly change the access to the descriptor was determined as write (writeonly was true) and the error was WRITE_AFTER_WRITE. After writeonly change the descriptor access is determined as read which results in WRITE_AFTER_READ.

Unfortunately current code does not have "no access" option, it assumes the resource is used and tries to classify it either as read or write. @spencer-lunarg Do you think it's possible to use IsReadFrom() in addition to IsWrittenTo to determine there are no accesses and in that case to skip hazard detection?

[spencer-lunarg]

@jrprice thanks for getting a very minimal case

This is the SPIR-V

The OpImageQuerySize is NOT an image read, but we there is still the OpLoad and I currently mark that as a read access so IsReadFrom() == true but IsImageReadFrom() == false here

[spencer-lunarg]

note %image = OpVariable %12 UniformConstant is layout(set = 0, binding = 0, rgba8) writeonly uniform image2D image;

[spencer-lunarg]

changing the shader (to avoid dead code elimination)

#version 450
layout(set = 0, binding = 0, rgba8) writeonly uniform image2D image;
layout(set = 0, binding = 1) buffer SSBO { uvec2 size; } data;
void main(){
    data.size = imageSize(image);
}

and running through Intel MESA, the final shader looks like

decl_var image INTERP_MODE_NONE restrict readonly writeonly r8g8b8a8_unorm image2D image (~0, 0, 0)
decl_var ssbo INTERP_MODE_NONE restrict writeonly SSBO data (~0, 0, 1)
decl_function main (0 params)

impl main {
    con block b0:      // preds: 
    con 32    %0 = load_const (0x00000000)
    con 32    %1 = load_const (0xdeaddead = -6264320714451451904.000000 = -559030611 = 3735936685)
    con 32    %2 = load_const (0x00000002 = 0.000000)
    con 32    %3 = @resource_intel (%1 (0xdeaddead), %2 (0x2), %0 (0x0), %0 (0x0)) (desc_set=0, binding=0, resource_intel=0, resource_block_intel=-1)
    con 32x2  %4 = @image_size (%3, %0 (0x0)) (image_dim=2D, image_array=false, format=r8g8b8a8_unorm, access=restrict|readonly|writeonly, range_base=0)
    con 32    %5 = load_const (0x00000003 = 0.000000)
    con 32    %6 = @resource_intel (%1 (0xdeaddead), %5 (0x3), %0 (0x0), %0 (0x0)) (desc_set=0, binding=1, resource_intel=0, resource_block_intel=-1)
                   @store_ssbo (%4, %6, %0 (0x0)) (wrmask=xy, access=writeonly, align_mul=1073741824, align_offset=0)
                   // succs: b1 
    block b1:
}

I can't find any documentation on resource_intel but I am assuming this is an access

[spencer-lunarg]

final update - got confirmation that

resource_intel reads the descriptor, not the image data

so for sync val that care about the data, this is should not be marked as a read, but for things like detecting if the descriptor was statically used this needs to marked as read