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

instrument: Use Import linkage for instrumentation functions #5355

Merged

Conversation

jeremyg-lunarg
Copy link
Contributor

@jeremyg-lunarg jeremyg-lunarg commented Aug 1, 2023

These functions are getting far too complicated to code in SPIRV-Tools C++. Replace them with import stubs so that the real implementations can live in Vulkan-ValidationLayers where they belong.

VVL will need to define these functions in spirv and link them to the instrumented version of the user's shader.

From here on out, VVL can redefine the functions and any data they use without updating SPIRV-Tools. Changing the function declarations will still require both VVL and SPIRV-Tools to be updated in lock step

@jeremyg-lunarg jeremyg-lunarg marked this pull request as draft August 1, 2023 17:55
@jeremyg-lunarg jeremyg-lunarg force-pushed the jeremyg-inst-linkage branch 4 times, most recently from 25e945a to 7ae0c4a Compare August 9, 2023 17:01
@jeremyg-lunarg jeremyg-lunarg changed the title instrument: Use Import linkage for inst_bindless_desc_check() instrument: Use Import linkage for instrumentation functions Aug 9, 2023
@jeremyg-lunarg jeremyg-lunarg force-pushed the jeremyg-inst-linkage branch 2 times, most recently from 98be43d to 5389112 Compare August 10, 2023 21:11
@jeremyg-lunarg jeremyg-lunarg force-pushed the jeremyg-inst-linkage branch 2 times, most recently from 3841a79 to b07c092 Compare August 22, 2023 20:13
@jeremyg-lunarg jeremyg-lunarg force-pushed the jeremyg-inst-linkage branch 2 times, most recently from 9aa263a to 55a19a8 Compare September 6, 2023 16:19
These functions are getting far too complicated to code in SPIRV-Tools
C++. Replace them with import stubs so that the real implementations
can live in Vulkan-ValidationLayers where they belong.

VVL will need to define these functions in spirv and link them to the
instrumented version of the user's shader.

From here on out, VVL can redefine the functions and any data they use
without updating SPIRV-Tools. Changing the function declarations will
still require both VVL and SPIRV-Tools to be updated in lock step.
@jeremyg-lunarg
Copy link
Contributor Author

This change will break Vulkan-ValidationLayers until KhronosGroup/Vulkan-ValidationLayers#6238 merges. I'm trying to get everything all ready to go, so that the break window is minimal. And we should have far less breaking changes from here on out.

Copy link
Collaborator

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

This is an API breaking change.
But you own both parts and want to do this, so I won't stand your way. :-)

Let us know when you want this merged.

Copy link
Contributor

@jeremy-lunarg jeremy-lunarg left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@jeremyg-lunarg jeremyg-lunarg merged commit ee7598d into KhronosGroup:main Sep 20, 2023
22 checks passed
@jeremyg-lunarg jeremyg-lunarg deleted the jeremyg-inst-linkage branch September 20, 2023 16:52
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.

None yet

3 participants