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.

Sign up for GitHub

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

Jump to bottom

Update spirv-tools version #5089

Merged
merged 11 commits into from
Sep 19, 2024
Merged

Update spirv-tools version #5089

merged 11 commits into from
Sep 19, 2024

Conversation

cheneym2
Copy link
Collaborator

No description provided.

@cheneym2 cheneym2 added the pr: non-breaking PRs without breaking changes label Sep 17, 2024
@cheneym2
Fix compute-derivative regressions with upgraded spirv headers
e2c6815 
Extension was promoted from NV to KHR while retaining same enums.

Fixes shader-slang#5106
@cheneym2
Prevent DCE on ray trace position fetch
2e09429 
Adds dummy usage to the intersection positions fetched
from HitTriangleVertexPositions to prevent DCE from
removing their usage.

Fixes shader-slang#5105
@jkwak-work
Copy link
Collaborator

I think you need to include external/spirv-tools-generated in this PR, isn't it?

@cheneym2
Copy link
Collaborator Author

I think you need to include external/spirv-tools-generated in this PR, isn't it?

Maybe, I'm curious if the PR passes without that, but in any case, I will check to see what differences exist in the generated files.

@cheneym2
Copy link
Collaborator Author

I was hasty and added the generated files to the PR before finding out if the checks would have passed. Anyways, PR is updated.

jkwak-work
jkwak-work previously approved these changes Sep 18, 2024
View reviewed changes
Copy link
Collaborator

@jkwak-work jkwak-work left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@jkwak-work
Copy link
Collaborator

I was hasty and added the generated files to the PR before finding out if the checks would have passed. Anyways, PR is updated.

I was curious about it too.
But to share my experience, I was getting a compile error when I didn't update the generated files. That's when I learned that external/spirv-tools-generated existed.
So you may get lucky without updating it for this time, but I am pretty sure it is not a proper way to upgrade the spirv-header.

@cheneym2
More DerivativeGroup*KHR test fixes
aa5f320 
Add fixes for a missing test intrinsic-derivative-function-in-compute.slang
Use the {{NV|KHR}} syntax to tolerate either enum.

Fixes shader-slang#5106
@csyonghe
Copy link
Collaborator

Yeah, I do think updating the generated files is needed.

@cheneym2 cheneym2 force-pushed the cheneym2/bump_spirv branch 12 times, most recently from 7c42d1d to 4db987a Compare September 19, 2024 16:18
@csyonghe csyonghe merged commit 9d40ce4 into shader-slang:master Sep 19, 2024
12 checks passed
@cheneym2 cheneym2 deleted the cheneym2/bump_spirv branch September 19, 2024 20:10
@jkwak-work jkwak-work mentioned this pull request Sep 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csyonghe csyonghe csyonghe approved these changes

@jkwak-work jkwak-work jkwak-work left review comments

Assignees
No one assigned
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cheneym2 @jkwak-work @csyonghe