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

Fix and/or intrinsics with non-int parameters #7060

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

Conversation

pow2clk
Copy link
Member

@pow2clk pow2clk commented Jan 13, 2025

And/or intrinsics were set to allow any parameters, which is consistent with the behavior of the && and || operators they were meant to replace, however this meant that if they were passed floating point values, those values would be applied to the binary and/or operands, which isn't allowed. Instead, they should be converted to booleans to be consistent with the behavior of && and ||. This can be done simply by restricting the parameters to booleans which forces the appropriate conversions. Adds tests for for using bools, ints, and floats in scalars, different sized vectors, and matrices as parameters to or and and.

Fixes: #7057
FIxes: #6995

And/or intrinsics were set to allow any parameters, which is consistent with the behavior of the && and || operators they were meant to replace, however this meant that if they were passed floating point values, those values would be applied to the binary and/or operands, which isn't allowed. Instead, they should be converted to booleans to be consistent with the behavior of && and ||. This can be done simply by restricting the parameters to booleans which forces the appropriate conversions.
Adds tests for for using bools, ints, and floats in scalars, different sized vectors, and matrices as parameters to or and and.

Fixes microsoft#7057
@pow2clk pow2clk requested a review from a team as a code owner January 13, 2025 12:14
@s-perron
Copy link
Collaborator

This might mean that #7055 is not need anymore.

@pow2clk
Copy link
Member Author

pow2clk commented Jan 13, 2025

This might mean that #7055 is not need anymore.

I think it should. That wasn't my intent! I caught this in an independent investigation, but I'm glad we can resolve something globally.

@pow2clk pow2clk added this to the Release 1.8.2502 milestone Jan 13, 2025
Copy link
Collaborator

@s-perron s-perron left a comment

Choose a reason for hiding this comment

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

Would it be possible to add SPIR-V tests? This seems to work for SPIR-V as well. However, I did find another bug with matrices, but that is a different issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
2 participants