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

fix scalar relational operator translation. #1753

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix scalar relational operator translation. #1753

wants to merge 1 commit into from

Conversation

airlied
Copy link
Contributor

@airlied airlied commented Dec 5, 2022

The CL spec for relational operators says that for scalar values the return value is 0/1 not 0/-1 which is for vectors.

Fix the translator to use 0/1 when it's a scalar operator

@airlied
Copy link
Contributor Author

airlied commented Dec 5, 2022

Fixes #1752

@airlied
fix scalar relational operator translation.
a3984e5 
The CL spec for relational operators says that for scalar values
the return value is 0/1 not 0/-1 which is for vectors.

Fix the translator to use 0/1 when it's a scalar operator.
@airlied airlied force-pushed the fix-relational-scalars branch from 6923b1b to a3984e5 Compare December 5, 2022 03:55
MrSidims
MrSidims approved these changes Dec 6, 2022
View reviewed changes
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.

Hi, thanks for the report and the PR!

Unlike OpenCL spec - SPIR-V says nothing about, what bits should be set, but I believe that the fix is correct. Not sure if it worth to add a test.
@svenvh please take a look.

@MrSidims
Copy link
Contributor

MrSidims commented Dec 6, 2022

Closing and reopening PR to restart tests

@MrSidims MrSidims closed this Dec 6, 2022
@MrSidims MrSidims reopened this Dec 6, 2022
@MrSidims MrSidims requested a review from svenvh December 7, 2022 12:20
@svenvh
Copy link
Member

svenvh commented Dec 7, 2022

LGTM, but I'd recommend adding/extending a test case.

@airlied
Copy link
Contributor Author

airlied commented Dec 12, 2022

@svenvh any pointers on what sort of test could be used here? I'm not really sure where the current code is tested if at all, so how would I test the return value here?

@svenvh
Copy link
Member

svenvh commented Dec 12, 2022

@svenvh any pointers on what sort of test could be used here? I'm not really sure where the current code is tested if at all, so how would I test the return value here?

The current pattern doesn't seem to be tested indeed (otherwise some tests would have failed with your change). I think test/transcoding/relationals_{half,float,double}.ll exercises this code path, so I would try adding CHECK lines for the new IR after the corresponding calls and Is*/SignBit ops.

MrSidims
MrSidims reviewed Apr 20, 2023
View reviewed changes
Copy link
Contributor

@MrSidims MrSidims left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thanks for adding the test! // Oops, wrong review, was look at this PR and in #1962 , can't delete the comment for some reasons

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrSidims MrSidims MrSidims approved these changes

@svenvh svenvh Awaiting requested review from svenvh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@airlied @MrSidims @svenvh