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

Load is moved past store with optimised SPIR-V input #638

Closed
rossy opened this issue Jul 9, 2018 · 4 comments
Closed

Load is moved past store with optimised SPIR-V input #638

rossy opened this issue Jul 9, 2018 · 4 comments
Labels
bug Feature which should work in SPIRV-Cross does not for some reason.

Comments

@rossy
Copy link
Contributor

rossy commented Jul 9, 2018

I'm trying to debug this issue in mpv. It seems like a load is being migrated past a store in the HLSL output, but only when the shader is compiled with optimisations and a recent version of shaderc.

For example, this GLSL shader:

#version 450 core

layout(std430, binding=0) buffer SSBO {
	uint index;
	uint array[64];
};

void main()
{
	uint value = array[index];
	for (uint i = 0; i < 64; i++)
		array[i] = 0;
	array[index] = value;
}

When compiled without optimisation:

glslc -std=450core -g -fshader-stage=compute --target-env=vulkan test.comp -o test.comp.spv

Produces this SPIR-V, which produces the following HLSL when compiled with SPIRV-Cross. This seems like the correct translation of the above shader:

RWByteAddressBuffer _13 : register(u0, space0);

void comp_main()
{
    uint value = _13.Load(_13.Load(0) * 4 + 4);
    for (uint i = 0u; i < 64u; i++)
    {
        _13.Store(i * 4 + 4, 0u);
    }
    _13.Store(_13.Load(0) * 4 + 4, value);
}

[numthreads(1, 1, 1)]
void main()
{
    comp_main();
}

When compiled with optimisation:

glslc -std=450core -g -Os -fshader-stage=compute --target-env=vulkan test.comp -o test.opt.comp.spv

It produces this SPIR-V, which also seems correct. The loads and stores are still in the right order. But SPIRV-Cross produces this HLSL, which moves one of the loads for the array past the store for the same array:

RWByteAddressBuffer _13 : register(u0, space0);

void comp_main()
{
    for (uint _40 = 0u; _40 < 64u; )
    {
        _13.Store(_40 * 4 + 4, 0u);
        _40++;
        continue;
    }
    _13.Store(_13.Load(0) * 4 + 4, _13.Load(_13.Load(0) * 4 + 4));
}

[numthreads(1, 1, 1)]
void main()
{
    comp_main();
}

This probably also happens with glslValidator/spirv-opt output, but I haven't tested those.

@HansKristian-Work HansKristian-Work added the needs triage Needs to be reproduced before it can become a different issue type. label Jul 9, 2018
@HansKristian-Work
Copy link
Contributor

Sounds like a simple lack of expression invalidation, but surprising it's not happening here.

@HansKristian-Work HansKristian-Work added bug Feature which should work in SPIRV-Cross does not for some reason. and removed needs triage Needs to be reproduced before it can become a different issue type. labels Jul 9, 2018
@HansKristian-Work
Copy link
Contributor

HansKristian-Work commented Jul 9, 2018

Ok, verified. This bug only seems to trigger on HLSL backend, which is a bit weird. On GLSL and MSL, the expression is properly invalidated.

@HansKristian-Work
Copy link
Contributor

Pretty dumb oversight after all.

rossy added a commit to rossy/crossc that referenced this issue Jul 9, 2018
This contains a fix for an issue with UAV dependency tracking.

See KhronosGroup/SPIRV-Cross#638
@rossy
Copy link
Contributor Author

rossy commented Jul 9, 2018

It works now. Thanks for the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Feature which should work in SPIRV-Cross does not for some reason.
Projects
None yet
Development

No branches or pull requests

2 participants