Skip to content

Commit

Permalink
Cache transform feedback varying names in the executable
Browse files Browse the repository at this point in the history 
Currently, ANGLE actually does a full link of the programs inside PPOs.
This was never the intention of the spec (hence why an explicit link
doesn't exist).  During this link operation, the transform feedback
varying names are used, and they are retrieved from the program itself.

This is not correct, because the transform feedback varyings may have
changed, the program may have failed to relink, and the program pipeline
is expected to continue functioning using the "installed" executable.

Bug: angleproject:5486
Bug: angleproject:8297
Change-Id: I583dbd2abcc51e8536b4c460b92211bdddebda16
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4834055
Reviewed-by: Charlie Lao <cclao@google.com>
Reviewed-by: Yuxin Hu <yuxinhu@google.com>
Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>
  • Loading branch information
@ShabbyX
ShabbyX authored and Angle LUCI CQ committed Sep 2, 2023
1 parent 179bd77 commit ebf1e71
Show file tree
Hide file tree
Showing 5 changed files with 88 additions and 60 deletions.
32 changes: 25 additions & 7 deletions src/libANGLE/Program.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1173,11 +1173,17 @@ angle::Result Program::linkImpl(const Context *context)
auto *platform = ANGLEPlatformCurrent();
double startTime = platform->currentTime(platform);

// Make sure the executable state is in sync with the program. Only the transform feedback
// buffer mode is duplicated in the executable as is the only link-input that is also needed at
// draw time.
// Make sure the executable state is in sync with the program.
//
// The transform feedback buffer mode is duplicated in the executable as is the only link-input
// that is also needed at draw time.
//
// The transform feedback varying names are duplicated because the program pipeline link is not
// currently able to use the link result of the program directly (and redoes the link, using
// these names).
mState.mExecutable->mPODStruct.transformFeedbackBufferMode =
mState.mTransformFeedbackBufferMode;
mState.mExecutable->mTransformFeedbackVaryingNames = mState.mTransformFeedbackVaryingNames;

// Unlink the program, but do not clear the validation-related caching yet, since we can still
// use the previously linked program if linking the shaders fails.
Expand Down Expand Up @@ -1335,10 +1341,9 @@ angle::Result Program::linkImpl(const Context *context)
}

mergedVaryings = GetMergedVaryingsFromLinkingVariables(linkingVariables);
if (!mState.mExecutable->linkMergedVaryings(
caps, limitations, clientVersion, isWebGL, mergedVaryings,
mState.mTransformFeedbackVaryingNames, linkingVariables, isSeparable(),
&resources.varyingPacking))
if (!mState.mExecutable->linkMergedVaryings(caps, limitations, clientVersion, isWebGL,
mergedVaryings, linkingVariables, isSeparable(),
&resources.varyingPacking))
{
return angle::Result::Continue;
}
Expand Down Expand Up @@ -3573,6 +3578,12 @@ angle::Result Program::serialize(const Context *context, angle::MemoryBuffer *bi
stream.writeBool(mState.mSeparable);
stream.writeInt(mState.mTransformFeedbackBufferMode);

stream.writeInt(mState.mTransformFeedbackVaryingNames.size());
for (const std::string &name : mState.mTransformFeedbackVaryingNames)
{
stream.writeString(name);
}

mState.mExecutable->save(mState.mSeparable, &stream);

// Warn the app layer if saving a binary with unsupported transform feedback.
Expand Down Expand Up @@ -3670,6 +3681,12 @@ angle::Result Program::deserialize(const Context *context, BinaryInputStream &st
mState.mSeparable = stream.readBool();
mState.mTransformFeedbackBufferMode = stream.readInt<GLenum>();

mState.mTransformFeedbackVaryingNames.resize(stream.readInt<size_t>());
for (std::string &name : mState.mTransformFeedbackVaryingNames)
{
name = stream.readString();
}

mState.mExecutable->load(mState.mSeparable, &stream);

static_assert(static_cast<unsigned long>(ShaderType::EnumCount) <= sizeof(unsigned long) * 8,
Expand All @@ -3686,6 +3703,7 @@ angle::Result Program::deserialize(const Context *context, BinaryInputStream &st
if (!mState.mAttachedShaders[ShaderType::Compute])
{
mState.mExecutable->updateTransformFeedbackStrides();
mState.mExecutable->mTransformFeedbackVaryingNames = mState.mTransformFeedbackVaryingNames;
}

if (context->getShareGroup()->getFrameCaptureShared()->enabled())
Expand Down
49 changes: 21 additions & 28 deletions src/libANGLE/ProgramExecutable.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -882,21 +882,18 @@ void ProgramExecutable::saveLinkedStateInfo(const ProgramState &state)
}
}

bool ProgramExecutable::linkMergedVaryings(
const Caps &caps,
const Limitations &limitations,
const Version &clientVersion,
bool webglCompatibility,
const ProgramMergedVaryings &mergedVaryings,
const std::vector<std::string> &transformFeedbackVaryingNames,
const LinkingVariables &linkingVariables,
bool isSeparable,
ProgramVaryingPacking *varyingPacking)
bool ProgramExecutable::linkMergedVaryings(const Caps &caps,
const Limitations &limitations,
const Version &clientVersion,
bool webglCompatibility,
const ProgramMergedVaryings &mergedVaryings,
const LinkingVariables &linkingVariables,
bool isSeparable,
ProgramVaryingPacking *varyingPacking)
{
ShaderType tfStage = GetLastPreFragmentStage(linkingVariables.isShaderStageUsedBitset);

if (!linkValidateTransformFeedback(caps, clientVersion, mergedVaryings, tfStage,
transformFeedbackVaryingNames))
if (!linkValidateTransformFeedback(caps, clientVersion, mergedVaryings, tfStage))
{
return false;
}
Expand Down Expand Up @@ -930,28 +927,26 @@ bool ProgramExecutable::linkMergedVaryings(
}

if (!varyingPacking->collectAndPackUserVaryings(*mInfoLog, caps, packMode, activeShadersMask,
mergedVaryings, transformFeedbackVaryingNames,
mergedVaryings, mTransformFeedbackVaryingNames,
isSeparable))
{
return false;
}

gatherTransformFeedbackVaryings(mergedVaryings, tfStage, transformFeedbackVaryingNames);
gatherTransformFeedbackVaryings(mergedVaryings, tfStage);
updateTransformFeedbackStrides();

return true;
}

bool ProgramExecutable::linkValidateTransformFeedback(
const Caps &caps,
const Version &clientVersion,
const ProgramMergedVaryings &varyings,
ShaderType stage,
const std::vector<std::string> &transformFeedbackVaryingNames)
bool ProgramExecutable::linkValidateTransformFeedback(const Caps &caps,
const Version &clientVersion,
const ProgramMergedVaryings &varyings,
ShaderType stage)
{
// Validate the tf names regardless of the actual program varyings.
std::set<std::string> uniqueNames;
for (const std::string &tfVaryingName : transformFeedbackVaryingNames)
for (const std::string &tfVaryingName : mTransformFeedbackVaryingNames)
{
if (clientVersion < Version(3, 1) && tfVaryingName.find('[') != std::string::npos)
{
Expand Down Expand Up @@ -982,7 +977,7 @@ bool ProgramExecutable::linkValidateTransformFeedback(
// From OpneGLES spec. 11.1.2.1: A program will fail to link if:
// the count specified by TransformFeedbackVaryings is non-zero, but the
// program object has no vertex, tessellation evaluation, or geometry shader
if (transformFeedbackVaryingNames.size() > 0 &&
if (mTransformFeedbackVaryingNames.size() > 0 &&
!gl::ShaderTypeSupportsTransformFeedback(getLinkedTransformFeedbackStage()))
{
*mInfoLog << "Linked transform feedback stage " << getLinkedTransformFeedbackStage()
Expand All @@ -992,7 +987,7 @@ bool ProgramExecutable::linkValidateTransformFeedback(

// Validate against program varyings.
size_t totalComponents = 0;
for (const std::string &tfVaryingName : transformFeedbackVaryingNames)
for (const std::string &tfVaryingName : mTransformFeedbackVaryingNames)
{
std::vector<unsigned int> subscripts;
std::string baseName = ParseResourceName(tfVaryingName, &subscripts);
Expand Down Expand Up @@ -1068,14 +1063,12 @@ bool ProgramExecutable::linkValidateTransformFeedback(
return true;
}

void ProgramExecutable::gatherTransformFeedbackVaryings(
const ProgramMergedVaryings &varyings,
ShaderType stage,
const std::vector<std::string> &transformFeedbackVaryingNames)
void ProgramExecutable::gatherTransformFeedbackVaryings(const ProgramMergedVaryings &varyings,
ShaderType stage)
{
// Gather the linked varyings that are used for transform feedback, they should all exist.
mLinkedTransformFeedbackVaryings.clear();
for (const std::string &tfVaryingName : transformFeedbackVaryingNames)
for (const std::string &tfVaryingName : mTransformFeedbackVaryingNames)
{
std::vector<unsigned int> subscripts;
std::string baseName = ParseResourceName(tfVaryingName, &subscripts);
Expand Down
25 changes: 14 additions & 11 deletions src/libANGLE/ProgramExecutable.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -529,22 +529,16 @@ class ProgramExecutable final : public angle::Subject
const Version &clientVersion,
bool webglCompatibility,
const ProgramMergedVaryings &mergedVaryings,
const std::vector<std::string> &transformFeedbackVaryingNames,
const LinkingVariables &linkingVariables,
bool isSeparable,
ProgramVaryingPacking *varyingPacking);

bool linkValidateTransformFeedback(
const Caps &caps,
const Version &clientVersion,
const ProgramMergedVaryings &varyings,
ShaderType stage,
const std::vector<std::string> &transformFeedbackVaryingNames);
bool linkValidateTransformFeedback(const Caps &caps,
const Version &clientVersion,
const ProgramMergedVaryings &varyings,
ShaderType stage);

void gatherTransformFeedbackVaryings(
const ProgramMergedVaryings &varyings,
ShaderType stage,
const std::vector<std::string> &transformFeedbackVaryingNames);
void gatherTransformFeedbackVaryings(const ProgramMergedVaryings &varyings, ShaderType stage);

void updateTransformFeedbackStrides();

Expand Down Expand Up @@ -661,6 +655,15 @@ class ProgramExecutable final : public angle::Subject
// Vertex attributes, Fragment input varyings, etc.
std::vector<ProgramInput> mProgramInputs;
std::vector<TransformFeedbackVarying> mLinkedTransformFeedbackVaryings;
// Duplicate of ProgramState::mTransformFeedbackVaryingNames. This is cached here because the
// xfb names may change, relink may fail, yet program pipeline link should be able to function
// with the last installed executable. In truth, program pipeline link should have been able to
// hoist transform feedback varyings directly from the executable, among most other things, but
// that is currently not done.
//
// This array is not serialized, it's already done by the program, and will be duplicated during
// deserialization.
std::vector<std::string> mTransformFeedbackVaryingNames;
// The size of the data written to each transform feedback buffer per vertex.
std::vector<GLsizei> mTransformFeedbackStrides;
// Uniforms are sorted in order:
Expand Down
8 changes: 4 additions & 4 deletions src/libANGLE/ProgramPipeline.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -540,12 +540,12 @@ angle::Result ProgramPipeline::link(const Context *context)
tfProgram = mState.mPrograms[ShaderType::Vertex];
}

const std::vector<std::string> &transformFeedbackVaryingNames =
tfProgram->getState().getTransformFeedbackVaryingNames();
mState.mExecutable->mTransformFeedbackVaryingNames =
tfProgram->getExecutable().mTransformFeedbackVaryingNames;

if (!mState.mExecutable->linkMergedVaryings(caps, limitations, clientVersion, isWebGL,
mergedVaryings, transformFeedbackVaryingNames,
linkingVariables, false, &varyingPacking))
mergedVaryings, linkingVariables, false,
&varyingPacking))
{
return angle::Result::Stop;
}
Expand Down
34 changes: 24 additions & 10 deletions src/tests/gl_tests/ProgramPipelineTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,16 @@ void ProgramPipelineXFBTest31::bindProgramPipelineWithXFBVaryings(
const std::vector<std::string> &tfVaryings,
GLenum bufferMode)
{
mVertProg = glCreateShaderProgramv(GL_VERTEX_SHADER, 1, &vertString);
ASSERT_NE(mVertProg, 0u);
GLShader vertShader(GL_VERTEX_SHADER);
mVertProg = glCreateProgram();

glShaderSource(vertShader, 1, &vertString, nullptr);
glCompileShader(vertShader);
glProgramParameteri(mVertProg, GL_PROGRAM_SEPARABLE, GL_TRUE);
glAttachShader(mVertProg, vertShader);
glLinkProgram(mVertProg);
EXPECT_GL_NO_ERROR();

mFragProg = glCreateShaderProgramv(GL_FRAGMENT_SHADER, 1, &fragString);
ASSERT_NE(mFragProg, 0u);

Expand Down Expand Up @@ -1245,21 +1253,20 @@ TEST_P(ProgramPipelineXFBTest31, VaryingIOBlockSeparableProgramWithXFB)
// Only the Vulkan backend supports PPOs
ANGLE_SKIP_TEST_IF(!IsVulkan());
ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_shader_io_blocks"));
// http://anglebug.com/5486
ANGLE_SKIP_TEST_IF(IsVulkan());

constexpr char kVS[] =
R"(#version 310 es
#extension GL_EXT_shader_io_blocks : require
precision highp float;
in vec4 inputAttribute;
out Block_inout { vec4 value; } user_out;
out Block_inout { vec4 value; vec4 value2; } user_out;
void main()
{
gl_Position = inputAttribute;
user_out.value = vec4(4.0, 5.0, 6.0, 7.0);
user_out.value2 = vec4(8.0, 9.0, 10.0, 11.0);
})";

constexpr char kFS[] =
Expand All @@ -1268,17 +1275,24 @@ TEST_P(ProgramPipelineXFBTest31, VaryingIOBlockSeparableProgramWithXFB)
precision highp float;
layout(location = 0) out mediump vec4 color;
in Block_inout { vec4 value; } user_in;
in Block_inout { vec4 value; vec4 value2; } user_in;
void main()
{
color = vec4(1, 0, 0, 1);
})";
std::vector<std::string> tfVaryings;
tfVaryings.push_back("Block_inout.value");
tfVaryings.push_back("Block_inout.value2");
bindProgramPipelineWithXFBVaryings(kVS, kFS, tfVaryings, GL_INTERLEAVED_ATTRIBS);
glBindBufferBase(GL_TRANSFORM_FEEDBACK_BUFFER, 0, mTransformFeedbackBuffer);

// Make sure reconfiguring the vertex shader's transform feedback varyings without a link does
// not affect the pipeline. Same with changing buffer modes
std::vector<const char *> tfVaryingsBogus = {"some", "invalid[0]", "names"};
glTransformFeedbackVaryings(mVertProg, static_cast<GLsizei>(tfVaryingsBogus.size()),
tfVaryingsBogus.data(), GL_SEPARATE_ATTRIBS);

glBeginTransformFeedback(GL_TRIANGLES);
drawQuadWithPPO("inputAttribute", 0.5f, 1.0f);
glEndTransformFeedback();
Expand All @@ -1287,11 +1301,11 @@ TEST_P(ProgramPipelineXFBTest31, VaryingIOBlockSeparableProgramWithXFB)
EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::red);

void *mappedBuffer =
glMapBufferRange(GL_TRANSFORM_FEEDBACK_BUFFER, 0, sizeof(float) * 4, GL_MAP_READ_BIT);
glMapBufferRange(GL_TRANSFORM_FEEDBACK_BUFFER, 0, sizeof(float) * 8, GL_MAP_READ_BIT);
ASSERT_NE(nullptr, mappedBuffer);

float *mappedFloats = static_cast<float *>(mappedBuffer);
for (unsigned int cnt = 0; cnt < 4; ++cnt)
for (unsigned int cnt = 0; cnt < 8; ++cnt)
{
EXPECT_EQ(4 + cnt, mappedFloats[cnt]);
}
Expand Down Expand Up @@ -1480,9 +1494,9 @@ void main()
my_FragColor = texture(tex, texCoord);
})";

std::array<GLColor, kWidth *kHeight> redColor = {
std::array<GLColor, kWidth * kHeight> redColor = {
{GLColor::red, GLColor::red, GLColor::red, GLColor::red}};
std::array<GLColor, kWidth *kHeight> greenColor = {
std::array<GLColor, kWidth * kHeight> greenColor = {
{GLColor::green, GLColor::green, GLColor::green, GLColor::green}};

// Create a red texture and bind to texture unit 0
Expand Down

0 comments on commit ebf1e71

Please sign in to comment.