8000 Make IRComparer consider nans to be less than non-nans. by alexreinking · Pull Request #6626 · halide/Halide · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Make IRComparer consider nans to be less than non-nans. #6626

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
Merged
merged 4 commits into from
Feb 22, 2022
Merged

Make IRComparer consider nans to be less than non-nans. #6626

merged 4 commits into from
Feb 22, 2022

Conversation

alexreinking
Copy link
Member
@alexreinking alexreinking commented Feb 21, 2022 •
edited
Loading

Our IRComparer's compare_scalar believed that all floating point values are totally ordered, which is not true. NaNs always compare false in ordering comparisons.

Fixes #6624

@alexreinking alexreinking changed the title Make IRComparer consider nans to be less than non-nans. Fixes #6624 Make IRComparer consider nans to be less than non-nans. Feb 21, 2022
@LebedevRI
Copy link
Contributor

Does this need any "is in strict fp mode" check?

@alexreinking
Copy link
Member Author

Does this need any "is in strict fp mode" check?

I don't believe so, no. I don't think we ever want to consider nan equal to anything but nan for the purposes of comparing IR nodes.

@alexreinking
Copy link
Member Author

Sorry, I just realized I forgot to actually use the updated target in the test... going to fix that now.

abadams
abadams reviewed Feb 21, 2022
View reviewed changes
abadams
abadams reviewed Feb 21, 2022
View reviewed changes
abadams
abadams reviewed Feb 21, 2022
View reviewed changes
@alexreinking
Copy link
Member Author

Failing test looks like a performance flake to me... what are your thoughts, @abadams?

steven-johnson
steven-johnson reviewed Feb 22, 2022
View reviewed changes
@alexreinking
Copy link
Member Author

Leaving this for someone else to merge or patch further.

@abadams abadams merged commit a4ed033 into main Feb 22, 2022
@alexreinking alexreinking deleted the bugfix/6624 branch February 22, 2022 05:37
@apartridge
Copy link

Thanks for fixing this issue! Would it be possible to make a 13.0.5 release with this fix?

@alexreinking
Copy link
Member Author

@apartridge - that would be possible, but my bandwidth is somewhat limited until mid-week next week.

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

@steven-johnson steven-johnson steven-johnson left review comments

@abadams abadams abadams approved these changes

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

Successfully merging this pull request may close these issues.

select statement that contains NaN value always returns NaN
5 participants
@alexreinking @LebedevRI @apartridge @steven-johnson @abadams
0