Skip to content

Conversation

@rjodinchr
Copy link
Contributor

@CLAassistant
Copy link

CLAassistant commented Dec 28, 2023

CLA assistant check
All committers have signed the CLA.

@rjodinchr
Copy link
Contributor Author

rjodinchr commented Jan 5, 2024

Also, please note that this PR introduces a dependency of spirv-tools on vulkan-headers

@rjodinchr rjodinchr marked this pull request as ready for review January 11, 2024 14:36
@rjodinchr rjodinchr requested a review from dneto0 January 11, 2024 14:37
@rjodinchr rjodinchr force-pushed the vksp branch 2 times, most recently from 0f44668 to 4108da3 Compare February 23, 2024 15:45
@dneto0 dneto0 requested a review from s-perron March 7, 2024 14:39
@dneto0
Copy link
Collaborator

dneto0 commented Mar 7, 2024

I haven't looked deeply. Requiring a depdency on vulkan-headers seems unfortunate.

@rjodinchr
Copy link
Contributor Author

rjodinchr commented Mar 8, 2024

I have removed the dependency on vulkan-headers.
I will keep the passes in another repository, but those changes are still needed for vksp passes to work

@rjodinchr rjodinchr changed the title add passes for vulkan-shader-profiler add support for vulkan-shader-profiler external passes Mar 8, 2024
// 32-bit integers.
return GetConstant(
int_type, {static_cast<uint32_t>(val >> 32), static_cast<uint32_t>(val)});
int_type, {static_cast<uint32_t>(val), static_cast<uint32_t>(val >> 32)});
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the right fix.

OpConstant says

Types 32 bits wide or smaller take one word. Larger types take multiple words, with low-order words appearing first.

Too bad this wasn't caught by a unit test. I won't make you write one now.

const uint32_t numCapabilities;
const spv::Capability* capabilities;
const spv_operand_type_t operandTypes[16]; // TODO: Smaller/larger?
const spv_operand_type_t operandTypes[40]; // vksp needs at least 40
Copy link
Collaborator

Choose a reason for hiding this comment

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

These tables are pretty dumb. I'd like to refactor them for compactness.

@rjodinchr rjodinchr requested a review from dneto0 March 8, 2024 18:03
@dneto0 dneto0 enabled auto-merge (squash) March 15, 2024 17:16
@dneto0 dneto0 merged commit f20663c into KhronosGroup:main Mar 15, 2024
@rjodinchr rjodinchr deleted the vksp branch March 16, 2024 09:39
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.

4 participants