Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Fix C4244 from MSVC in sort. #966

Merged
merged 1 commit into from
Jun 9, 2020
Merged

Fix C4244 from MSVC in sort. #966

merged 1 commit into from
Jun 9, 2020

Conversation

zhihaoy
Copy link
Contributor

@zhihaoy zhihaoy commented Apr 1, 2019

No description provided.

@zhihaoy
Copy link
Contributor Author

zhihaoy commented Apr 17, 2019

The issue goes away in MSVC 19.16.27030.1, should I leave this P/R open?

@brycelelbach
Copy link
Collaborator

@zhihaoy, hrm, your call. This probably isn't the right fix; instead of casting, we should change the type of the variable. @griwes, thoughts?

@zhihaoy
Copy link
Contributor Author

zhihaoy commented Oct 28, 2019

It's a safe fix because "log2 round to integer" can't overflow any type. Raising num_passes to Size looks pedantic... I will leave it open for a while because someone else seems to encounter the same issue https://devtalk.nvidia.com/default/topic/1064002/b/t/post/5387873/

@alliepiper
Copy link
Collaborator

This patch looks reasonable to me -- we wouldn't be running more than 2^32 passes so just casting to an int should be ok. @brycelelbach @griwes if you have any objections to this let me know, but I'll start moving towards integrating this in case folks are still using older MSVC 2017 builds.

@alliepiper alliepiper added the testing: gpuCI passed Passed gpuCI testing. label May 28, 2020
@alliepiper
Copy link
Collaborator

Started internal CI: 28483968

@alliepiper alliepiper added testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). testing: internal ci passed Passed internal NVIDIA CI (DVS). and removed testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). labels Jun 1, 2020
@alliepiper alliepiper self-assigned this Jun 3, 2020
@alliepiper
Copy link
Collaborator

Tests pass -- I'll merge this soon.

@alliepiper alliepiper merged commit ee57582 into NVIDIA:master Jun 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alliepiper alliepiper alliepiper approved these changes

Assignees

@alliepiper alliepiper

Labels
testing: gpuCI passed Passed gpuCI testing. testing: internal ci passed Passed internal NVIDIA CI (DVS).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zhihaoy @brycelelbach @alliepiper