Add type-safe, const-safe loops to c10#46414
Conversation
💊 CI failures summary and remediationsAs of commit 332349eab5 (more details on the Dr. CI page): Commit 332349eab5 was recently pushed. Waiting for builds... This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 1 times. |
|
This pull request was exported from Phabricator. Differential Revision: D24334732 |
💊 CI failures summary and remediationsAs of commit e7aa64d (more details on the Dr. CI page):
❄️ 1 failure tentatively classified as flakybut reruns have not yet been triggered to confirm:
|
332349e to
6b6f13f
Compare
|
This pull request was exported from Phabricator. Differential Revision: D24334732 |
6b6f13f to
a6490b9
Compare
|
This pull request was exported from Phabricator. Differential Revision: D24334732 |
|
I'm going to let @smessmer make the call on this one. This seems to be a Boost-ism, not part of upcoming C++ standard library. |
|
That's correct, @ezyang, C++20 or prior doesn't, as far as I can tell, have a similar functionality. I'll edit the commit message to include:
|
|
This pull request was exported from Phabricator. Differential Revision: D24334732 |
a6490b9 to
3dc41a3
Compare
|
This pull request was exported from Phabricator. Differential Revision: D24334732 |
3dc41a3 to
5dbeb99
Compare
|
This pull request was exported from Phabricator. Differential Revision: D24334732 |
5dbeb99 to
0389708
Compare
Codecov Report
@@ Coverage Diff @@
## master #46414 +/- ##
=======================================
Coverage 80.64% 80.64%
=======================================
Files 1907 1907
Lines 206825 206828 +3
=======================================
+ Hits 166791 166799 +8
+ Misses 40034 40029 -5 |
ngimel
left a comment
There was a problem hiding this comment.
I'm not sure how to make it performance-neutral compared to (also type-safe) loops that are written with decltype. TORCH_CHECK is known to prevent inlining, and probably it would have to be added to an upper-limit variant too.
There was a problem hiding this comment.
This is adding a TORCH_CHECK and might affect performance of hot paths
There was a problem hiding this comment.
What would happen if end is negative? Can you add a test for this?
Summary: Pull Request resolved: pytorch#46414 For loops are often written with mismatched data types which causes silent type and sign coercion in the absence of integer conversion warnings. Getting around this in templated code requires convoluted patterns such as ``` for(auto i=decltype(var){0};i<var;i++) ``` with this diff we can instead write ``` for(const auto i = c10::irange(var)) ``` Note that this loop is type-safe and const-safe. The function introduced here (`c10::irange`) allows for type-safety and const-ness within for loops, which prevents the accidental truncation or modification of integers and other types, improving code safety. Test Plan: ``` buck test //caffe2/c10:c10_test_0 ``` Differential Revision: D24334732 fbshipit-source-id: 595d03de995288da83d0e4325e269511a7f122f7
|
This pull request was exported from Phabricator. Differential Revision: D24334732 |
0389708 to
e7aa64d
Compare
|
For the record, here's godbolt for irange compared to regular for loop, there's an extra cmovsq: https://godbolt.org/z/3o1968 |
|
This pull request has been merged in 8e74024. |
Summary: Pull Request resolved: pytorch#46414 For loops are often written with mismatched data types which causes silent type and sign coercion in the absence of integer conversion warnings. Getting around this in templated code requires convoluted patterns such as ``` for(auto i=decltype(var){0};i<var;i++) ``` with this diff we can instead write ``` for(const auto i = c10::irange(var)) ``` Note that this loop is type-safe and const-safe. The function introduced here (`c10::irange`) allows for type-safety and const-ness within for loops, which prevents the accidental truncation or modification of integers and other types, improving code safety. Test Plan: ``` buck test //caffe2/c10:c10_test_0 ``` Reviewed By: ngimel Differential Revision: D24334732 fbshipit-source-id: fec5ebda3643ec5589f7ea3a8e7bbea4432ed771
Pull Request resolved: #46414
For loops are often written with mismatched data types which causes silent type and sign coercion in the absence of integer conversion warnings. Getting around this in templated code requires convoluted patterns such as
(the above pattern appears ~31 times in the code base) with this diff we can instead write
Note that this loop is type-safe and const-safe. (Type-unsafe loops occur an unknown number of times do to the lack of conversion warnings on compilation.)
The functions introduced here (
c10::ranges::intsandc10::ranges::indices) allow for type-safety and const-ness within for loops, which prevents the accidental truncation or modification of integers and other types, improving code safety. Further, they use the same naming convention as the ranges-v3 library and offer an easy migration path to C++20 functionality.Test Plan:
Differential Revision: D24334732