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

MSL: Terminate function with return value using return if ending in unreachable. #2436

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

squidbus
Copy link
Contributor

@squidbus squidbus commented Jan 20, 2025

Fix for #2435.

If a function with return value ends in unreachable, insert a return statement to avoid MSL compile errors. This condition already exists for blocks ending in Kill, it just needs to be extended to cover Unreachable as well.

@squidbus
Copy link
Contributor Author

squidbus commented Jan 20, 2025

I had included some unrelated updates to other test files from what the test script generated, but for some reason they're just generating different output on my machine than CI. So I removed them.

%44 = OpAccessChain %_ptr_StorageBuffer_int %buff %int_0 %41
OpStore %44 %int_1
OpDemoteToHelperInvocation
OpUnreachable
Copy link
Contributor

Choose a reason for hiding this comment

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

This OpUnreachable is completely bogus fwiw. Having Unreachable post-dominate the entry block means that the entire shader is UB to execute, but ... whatever.

Copy link
Contributor Author

@squidbus squidbus Jan 23, 2025

Choose a reason for hiding this comment

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

Yes I had the same suspicion and will probably have to visit the generation that results in this in my use case. But it seems to function elsewhere (I think just because discarding eliminates potential side effects?) and I figured also that, while the spec says that executing OpUnreachable is UB, there is no indication that it should fail to compile.

Copy link
Contributor

@HansKristian-Work HansKristian-Work left a comment

Choose a reason for hiding this comment

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

Questionable, but ultimately harmless.

@HansKristian-Work HansKristian-Work merged commit 597881e into KhronosGroup:main Jan 23, 2025
10 checks passed
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.

2 participants