GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-100239: Handle NaN and zero division in guards by Eclips4 · Pull Request #128963 · python/cpython

Conversation

@Eclips4

@Eclips4 Eclips4 commented Jan 17, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Specialize long tail of binary operations using a table. #100239

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Eclips4
Fix zero division
8547393
@Eclips4 Eclips4 added the skip news label Jan 17, 2025
@Eclips4
Eclips4 requested a review from iritkatriel January 17, 2025 21:39
@bedevere-app bedevere-app Bot mentioned this pull request Jan 17, 2025
Specialize long tail of binary operations using a table. #100239
Open
@bedevere-app bedevere-app Bot added the awaiting core review label Jan 17, 2025
iritkatriel
iritkatriel reviewed Jan 17, 2025
View reviewed changes
Comment thread Lib/test/test_opcache.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Eclips4
Reorder calls
131b92a
@iritkatriel

iritkatriel commented Jan 17, 2025

Copy link
Copy Markdown
Member

Shall we check for NaN as well?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Eclips4

Eclips4 commented Jan 17, 2025

Copy link
Copy Markdown
Member Author

Shall we check for NaN as well?

You mean "Shall we continue with specialization if we encounter a NaN value?"?
If so, I think we should add a check for NaN to all guards because I don't see any benefit in specializing to get a NaN result (As far as I remember, any operation involving NaN results in NaN).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel

iritkatriel commented Jan 17, 2025

Copy link
Copy Markdown
Member

Shall we check for NaN as well?

You mean "Shall we continue with specialization if we encounter a NaN value?"? If so, I think we should add a check for NaN to all guards because I don't see any benefit in specializing to get a NaN result (As far as I remember, any operation involving NaN results in NaN).

Yes, I guess it would be fine. Might be good to cover this in the tests though.

Eclips4 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Eclips4
Address Irit's review && Add guards for NaN
8363e9d
@Eclips4 Eclips4 changed the title gh-100239: Handle zero division in guards gh-100239: Handle NaN and zero division in guards Jan 17, 2025
Eclips4
Eclips4 commented Jan 17, 2025
View reviewed changes
Comment thread Lib/test/test_opcache.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Eclips4
Eclips4 requested a review from iritkatriel January 17, 2025 22:58
tomasr8
tomasr8 reviewed Jan 17, 2025
View reviewed changes
Comment thread Python/specialize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel
iritkatriel approved these changes Jan 17, 2025
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Jan 17, 2025
@iritkatriel

iritkatriel commented Jan 17, 2025

Copy link
Copy Markdown
Member

Doesn't need to be in this PR, but I don't think we have a test for deoptimizing BINARY_OP_EXTEND. We should.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Eclips4 and others added 2 commits January 18, 2025 12:35
@Eclips4 @tomasr8
Update Python/specialize.c
850a862
Co-authored-by: Tomas R. <[email protected]>
@Eclips4
Apply the same technique for compactlong_float
f98667c
@Eclips4

Eclips4 commented Jan 18, 2025

Copy link
Copy Markdown
Member Author

Unfortunately ./python.exe -m test -v test_opcache -m test_binary_op -R 3:3 is failing:

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: FAILURE

I'm digging into what's happening.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Eclips4
I guess it should work
27cd99a
@Eclips4

Eclips4 commented Jan 18, 2025

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Eclips4
Eclips4 requested a review from iritkatriel January 18, 2025 11:46
iritkatriel
iritkatriel approved these changes Jan 18, 2025
View reviewed changes
Comment thread Python/specialize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/specialize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/specialize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/specialize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_opcache.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Eclips4 and others added 2 commits January 19, 2025 11:48
@Eclips4 @iritkatriel
Apply suggestions from code review
aa416bb
Co-authored-by: Irit Katriel <[email protected]>
@Eclips4
Address Irit's review
127cb30
@Eclips4
Eclips4 enabled auto-merge (squash) January 19, 2025 10:42
iritkatriel
iritkatriel approved these changes Jan 19, 2025
View reviewed changes

@iritkatriel iritkatriel left a comment

Copy link
Copy Markdown
Member

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.

Thank you!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Eclips4

Eclips4 commented Jan 19, 2025

Copy link
Copy Markdown
Member Author

Thank you Irit and Tomas for your review!

tomasr8 reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Eclips4
Eclips4 merged commit 6c52ada into python:main Jan 19, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Jan 19, 2025
@Eclips4
Eclips4 deleted the fix-zero-division branch January 19, 2025 14:16
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 21, 2025
@Eclips4 @tomasr8
@iritkatriel @srinivasreddy
pythongh-100239: Handle NaN and zero division in guards for `BINARY_O…
b70dbd4
…P_EXTEND` (python#128963)

Co-authored-by: Tomas R. <[email protected]>
Co-authored-by: Irit Katriel <[email protected]>
@chris-eibl chris-eibl mentioned this pull request Feb 14, 2026
gh-100239: use PyFloat_AS_DOUBLE and _PyLong_IsZero in the float / compactlong specializations #144826
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tomasr8 tomasr8 tomasr8 left review comments
@iritkatriel iritkatriel iritkatriel approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@Eclips4 @iritkatriel @tomasr8
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.