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

Emit OpModfStruct for GLSL EOpModf instead of OpModf #3757

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

chyyran
Copy link
Contributor

@chyyran chyyran commented Oct 7, 2024

My attempt at fixing #3756.

Also updated the tests to account for the new type getting added.

@CLAassistant
Copy link

CLAassistant commented Oct 7, 2024

CLA assistant check
All committers have signed the CLA.

@chyyran chyyran changed the title Emit OpModfStruct for GLSL EOpModf instead of OpModf (fixes #3756) Emit OpModfStruct for GLSL EOpModf instead of OpModf Oct 7, 2024
@arcady-lunarg arcady-lunarg self-requested a review October 8, 2024 19:03
@chyyran

This comment was marked as resolved.

@chyyran chyyran force-pushed the emit-modfstruct branch 2 times, most recently from 6511ab2 to ec04400 Compare October 8, 2024 22:41
@chyyran
Copy link
Contributor Author

chyyran commented Oct 8, 2024

Managed to get the runtests tests to run and updated those results as well.

@chyyran
Copy link
Contributor Author

chyyran commented Oct 14, 2024

@arcady-lunarg any updates on getting this merged?

Copy link
Contributor

@arcady-lunarg arcady-lunarg left a comment

Choose a reason for hiding this comment

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

The code actually looks fine, I don't really like the way the comments are worded though, could you please reword them?

assert(builder.isFloatType(builder.getScalarTypeId(typeId0)));
int width = builder.getScalarTypeWidth(typeId0);
if (width == 16)
// Using 16-bit whole number operand, enable extension E_SPV_AMD_gpu_shader_half_float
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be removed, the code is kind of self-explanatory

if (width == 16)
// Using 16-bit whole number operand, enable extension E_SPV_AMD_gpu_shader_half_float
builder.addExtension(spv::E_SPV_AMD_gpu_shader_half_float);
// The two members of the returned struct and x must all be the same type.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should maybe be more like "The returned struct has two members of the same type as the first argument"

@chyyran
Copy link
Contributor Author

chyyran commented Oct 21, 2024

@arcady-lunarg addressed nits as of 5c784d6

@chyyran chyyran requested a review from arcady-lunarg October 21, 2024 23:35
@arcady-lunarg arcady-lunarg added the kokoro:run Trigger Google bot runs label Oct 21, 2024
@kokoro-team kokoro-team removed the kokoro:run Trigger Google bot runs label Oct 21, 2024
@arcady-lunarg arcady-lunarg merged commit 3454c36 into KhronosGroup:main Oct 25, 2024
34 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.

4 participants