-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Fix IValue from SymBool on big-endian system #163647
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 IValue from SymBool on big-endian system #163647
Conversation
It fails both on s390x and x86_64 at least under some circumstances. Disable it for now until on s390x until it works reliably.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/163647
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit 28caf37 with merge base 1a42656 ( BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
9a1f383
to
d211950
Compare
When bool value is assigned as as_int on big endian systems, value of 1 updates last byte of int value. as_bool checks first byte of same memory location. That byte is not updated. If bool value was 'true', after such conversion the 'toBool' function would actually return 'false'. This change fixes multiple tests in test_nestedtensor.py such as test_as_nested_tensor_from_tensor_dim_2_layout_jagged_requires_grad_False_contiguous_False_cpu_float16
payload.u.as_int = *mi; | ||
#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ | ||
/* due to byteorder if value assigned as_int, as_bool actually is not set correctly */ | ||
payload.u.as_bool = *mi; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't just set as_bool
for every case due to
pytorch/aten/src/ATen/core/ivalue.h
Lines 683 to 690 in 39c340e
#if defined(__clang__) && defined(__x86_64__) | |
// Initializing entire payload stops valgrind's from reporting | |
// "jump or move depends on uninitialised value" in IValue copy constructor | |
// See https://github.com/pytorch/pytorch/issues/37117 | |
payload.u.as_int = b; | |
#else | |
payload.u.as_bool = b; | |
#endif |
I can remove these checks and just set as_bool
value for every case if that would be preferred way to fix this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@pytorchbot merge |
@pytorchbot merge -f "This looks fine" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Skip test_compiled_autograd_attribution on s390x It fails both on s390x and x86_64 at least under some circumstances. Disable it for now until on s390x until it works reliably. Pull Request resolved: pytorch#163647 Approved by: https://github.com/malfet
Skip test_compiled_autograd_attribution on s390x
It fails both on s390x and x86_64 at least under some circumstances. Disable it for now until on s390x until it works reliably.
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames @Lucaskabela