Skip to content

Set entry point names during pipeline creation#577

Merged
alan-baker merged 4 commits intogoogle:masterfrom
alan-baker:non-default-entry
Jul 15, 2019
Merged

Set entry point names during pipeline creation#577
alan-baker merged 4 commits intogoogle:masterfrom
alan-baker:non-default-entry

Conversation

@alan-baker
Copy link
Collaborator

Previously pipeline names were only set if there was an entry point command executed. This doesn't match up with how AmberScript specifies entry point names.

Now, if the entry point name was specified in the attach command, it will get set at pipeline creation.

* When vulkan::Pipeline is created set the entry point names for all
shaders that have had the entry point specified
  * add a test that previously failed
@alan-baker alan-baker requested review from dj2, dneto0 and jaebaek July 15, 2019 18:31
@alan-baker alan-baker self-assigned this Jul 15, 2019
Copy link
Contributor

@jaebaek jaebaek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a nit.

if (!r.IsSuccess())
return r;
const auto& name = shader_info.GetEntryPoint();
if (!name.empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is empty, it would be better to just return an error here?

if (name.empty())
  return Result("Vulkan: pipeline entry point is empty");
info.vk_pipeline->SetEntryPointName(stage, name);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is valid to not set an entry name if you plan to use the default name.

@alan-baker alan-baker merged commit 7bdced0 into google:master Jul 15, 2019

END

BUFFER in_buf DATA_TYPE uint32 DATA
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future note, this could have been a single line:

BUFFER in_buf DATA_TYPE uint32 DATA 9 END


RUN my_pipeline 1 1 1

EXPECT out_buf EQ_BUFFER in_buf
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, nice to see EQ_BUFFER being useful for tests. @hevrard

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad to see this too! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants