gh-100239: Handle NaN and zero division in guards#128963
Merged
Eclips4 merged 8 commits intopython:mainfrom Jan 19, 2025
Merged
gh-100239: Handle NaN and zero division in guards#128963Eclips4 merged 8 commits intopython:mainfrom
Eclips4 merged 8 commits intopython:mainfrom
Conversation
iritkatriel
reviewed
Jan 17, 2025
Member
|
Shall we check for NaN as well? |
Member
Author
You mean "Shall we continue with specialization if we encounter a NaN value?"? |
Member
Yes, I guess it would be fine. Might be good to cover this in the tests though. |
Eclips4
changed the title
Eclips4
commented
Jan 17, 2025
tomasr8
reviewed
Jan 17, 2025
iritkatriel
approved these changes
Jan 17, 2025
Member
|
Doesn't need to be in this PR, but I don't think we have a test for deoptimizing |
Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
Member
Author
|
Unfortunately Details== CPython 3.14.0a4+ (heads/fix-zero-division-dirty:850a862a7d8, Jan 18 2025, 13:17:14) [Clang 16.0.0 (clang-1600.0.26.4)]
== macOS-15.1.1-arm64-arm-64bit-Mach-O little-endian
== Python build: debug
== cwd: /Users/eclips4/programming/programming-languages/cpython/build/test_python_worker_4826æ
== CPU count: 8
== encodings: locale=UTF-8 FS=utf-8
== resources: all test resources are disabled, use -u option to unskip tests
Using random seed: 548978877
0:00:00 load avg: 2.19 Run 1 test sequentially in a single process
0:00:00 load avg: 2.19 [1/1] test_opcache
beginning 6 repetitions. Showing number of leaks (. for 0 or less, X for 10 or more)
123:456
test_binary_op (test.test_opcache.TestSpecializer.test_binary_op) ... ok
----------------------------------------------------------------------
Ran 1 test in 0.006s
OK
Xtest_binary_op (test.test_opcache.TestSpecializer.test_binary_op) ... ok
----------------------------------------------------------------------
Ran 1 test in 0.007s
OK
Xtest_binary_op (test.test_opcache.TestSpecializer.test_binary_op) ... FAIL
======================================================================
FAIL: test_binary_op (test.test_opcache.TestSpecializer.test_binary_op)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/eclips4/programming/programming-languages/cpython/Lib/test/test_opcache.py", line 1405, in test_binary_op
binary_op_nan()
~~~~~~~~~~~~~^^
File "/Users/eclips4/programming/programming-languages/cpython/Lib/test/test_opcache.py", line 1402, in binary_op_nan
self.assert_no_opcode(compactlong_lhs, "BINARY_OP_EXTEND")
~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/eclips4/programming/programming-languages/cpython/Lib/test/test_opcache.py", line 42, in assert_no_opcode
self.assertNotIn(opname, opnames)
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^
AssertionError: 'BINARY_OP_EXTEND' unexpectedly found in {'RESUME_CHECK', 'RETURN_VALUE', 'POP_TOP', 'LOAD_CONST_IMMORTAL', 'BINARY_OP_EXTEND', 'LOAD_SMALL_INT', 'LOAD_FAST'}
----------------------------------------------------------------------
Ran 1 test in 0.012s
FAILED (failures=1)
test test_opcache failed
test_opcache failed (1 failure)
== Tests result: FAILURE ==
1 test failed:
test_opcache
Total duration: 174 ms
Total tests: run=1 (filtered) failures=1
Total test files: run=1/1 (filtered) failed=1
Result: FAILUREI'm digging into what's happening. |
Member
Author
|
Okay, It seems that to get de-specialization it need to "hit" more guards. I'm not sure, though, if this is right. |
iritkatriel
approved these changes
Jan 18, 2025
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
Member
Author
|
Thank you Irit and Tomas for your review! |
srinivasreddy
pushed a commit
to srinivasreddy/cpython
that referenced
this pull request
Jan 21, 2025
…P_EXTEND` (python#128963) Co-authored-by: Tomas R. <tomas.roun8@gmail.com> Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.