Skip to content

gh-131798: Use sym_new_type instead of sym_new_not_null for _BUILD_LIST, _BUILD_SLICE, and _BUILD_MAP #132434

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 16, 2025
Merged

gh-131798: Use sym_new_type instead of sym_new_not_null for _BUILD_LIST, _BUILD_SLICE, and _BUILD_MAP #132434

merged 5 commits into from
Apr 16, 2025

Conversation

Zheaoli
Copy link
Contributor

@Zheaoli Zheaoli commented Apr 12, 2025 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Apr 12, 2025
Open
@TeamSpen210
Copy link

_BUILD_MAP returns a dict not a map() object.

Fidget-Spinner
Fidget-Spinner approved these changes Apr 12, 2025
View reviewed changes
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Apr 12, 2025

@Fidget-Spinner I think I still have some work for this PR

A few days ago, In #132289, @brandtbucher setup optimization path for _GUARD_NOS_DICT/_GUARD_NOS_LIST, But after using sym_new_type instead of sym_new_not_null, the _GUARD_NOS_DICT/_GUARD_NOS_LIST is not existed in the final generate result. So the test failed here https://github.com/python/cpython/actions/runs/14419426452/job/40440141201?pr=132434#step:4:1788

I'm not sure this is what we want to get(I guess it's, so maybe we just need to patch the test?)

@brandtbucher brandtbucher changed the title gh-131798: Use sym_new_type instead of sym_new_not_null for _BUILD_LIST, _BUILD_SET, _BUILD_MAP gh-131798: Use sym_new_type instead of sym_new_not_null for _BUILD_LIST, _BUILD_SLICE, _BUILD_MAP Apr 12, 2025
@brandtbucher brandtbucher changed the title gh-131798: Use sym_new_type instead of sym_new_not_null for _BUILD_LIST, _BUILD_SLICE, _BUILD_MAP gh-131798: Use sym_new_type instead of sym_new_not_null for _BUILD_LIST, _BUILD_SLICE, and _BUILD_MAP Apr 12, 2025
@brandtbucher
Copy link
Member

brandtbucher commented Apr 12, 2025 •
edited
Loading

@Zheaoli, that's actually a good thing! The additional information that you've added to the optimizer for _BUILD_MAP and _BUILD_LIST means that the JIT is able to remove those guard instructions. You should just update the tests to check for 0 instead of 1, since these guard instructions shouldn't be present at all anymore.

(I don't think writing a test for the _BUILD_SLICE change is going to be very helpful, given how this instruction is used right now. So don't worry about that one.)

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Apr 12, 2025 •
edited
Loading

that's actually a good thing! The additional information that you've added to the optimizer for _BUILD_MAP and _BUILD_LIST means that the JIT is able to remove those guard instructions. You should just update the tests to check for 0 instead of 1, since these guard instructions shouldn't be present at all anymore.

Yes, I guess the _GUARD_NOS_DICT is not needed here(just for the test) when we setup extra information for ops when I see the failed test. You confirmed my guess. Updating the PR

@Zheaoli Zheaoli marked this pull request as ready for review April 12, 2025 15:34
@Zheaoli Zheaoli force-pushed the manjusaka/build-tuple-set-map-list branch from 5101fc8 to 1e69348 Compare April 12, 2025 15:35
Fidget-Spinner
Fidget-Spinner reviewed Apr 14, 2025
View reviewed changes
Lib/test/test_capi/test_opt.py
@@ -1706,7 +1706,7 @@ def f(n):
self.assertEqual(res, 2 * TIER2_THRESHOLD)
self.assertIsNotNone(ex)
uops = get_opnames(ex)
self.assertEqual(uops.count("_GUARD_NOS_LIST"), 1)
self.assertEqual(uops.count("_GUARD_NOS_LIST"), 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please update the comment above and change Guarded to Unguarded. The guard has now been removed.

Same for test_remove_guard_for_known_type_dict

Zheaoli added 5 commits April 16, 2025 22:02
@Zheaoli
pythongh-131798: Use sym_new_type instead of sym_new_not_null for…
9be0d7f 
… _BUILD_LIST, _BUILD_SET, _BUILD_MAP

Signed-off-by: Manjusaka <[email protected]>
@Zheaoli
Fix review idea
c3d7c33 
Signed-off-by: Manjusaka <[email protected]>
@Zheaoli
Fix review idea
a724d89 
Signed-off-by: Manjusaka <[email protected]>
@Zheaoli
Update the test
0e12c85 
Signed-off-by: Manjusaka <[email protected]>
@Zheaoli
fix review idea
839a749 
Signed-off-by: Manjusaka <[email protected]>
@Zheaoli Zheaoli force-pushed the manjusaka/build-tuple-set-map-list branch from 8d55bc4 to 839a749 Compare April 16, 2025 14:02
@Fidget-Spinner
Copy link
Member

@Zheaoli please don't force push, just normal push so we can see the commit diff. We will squash and merge at the end as one commit. Thanks!

Fidget-Spinner
Fidget-Spinner approved these changes Apr 16, 2025
View reviewed changes
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again! Great job.

@Fidget-Spinner Fidget-Spinner merged commit b9e88ff into python:main Apr 16, 2025
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Zheaoli @TeamSpen210 @brandtbucher @Fidget-Spinner