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

Rewrite OpenCL explicit conversion builtins handling #2464

Merged
merged 2 commits into from
Apr 4, 2024

Conversation

vmaksimo
Copy link
Contributor

@vmaksimo vmaksimo commented Mar 27, 2024

Check that builtin is valid mostly by matching the regular expression - remove unneeded checks.
This is a follow-up to #2443

Check that builtin is valid mostly by matching the regular expression
@vmaksimo vmaksimo closed this Mar 28, 2024
@vmaksimo vmaksimo reopened this Mar 28, 2024
@vmaksimo
Copy link
Contributor Author

Please take a look @MrSidims @LU-JOHN @asudarsa @bwlodarcz @VyacheslavLevytskyy

lib/SPIRV/OCLToSPIRV.cpp Outdated Show resolved Hide resolved
std::smatch DestTyMatch;
if (!std::regex_match(TargetTyName, DestTyMatch, Expr))
if (!std::regex_match(ConversionFunc, DestTyMatch, Expr))
return;

// The first sub_match is the whole string; the next
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 might need to be rewritten and also appear before the line defining DestTy?

Thanks

lib/SPIRV/OCLToSPIRV.cpp Outdated Show resolved Hide resolved
lib/SPIRV/OCLToSPIRV.cpp Outdated Show resolved Hide resolved
lib/SPIRV/OCLToSPIRV.cpp Outdated Show resolved Hide resolved
@asudarsa
Copy link
Contributor

Test fail reported here: #2467
I am seeing it in my CI runs as well.

Thanks

Copy link
Contributor

@LU-JOHN LU-JOHN left a comment

Choose a reason for hiding this comment

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

LGTM

@MrSidims MrSidims requested a review from asudarsa April 3, 2024 10:38
Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

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

LGTM, @asudarsa were your questions addressed?

@MrSidims MrSidims merged commit 0414482 into KhronosGroup:main Apr 4, 2024
9 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.

6 participants