Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-126902: Strength reduce _CHECK_FUNCTION#126856

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
Fidget-Spinner wants to merge6 commits intopython:main
base:main
Choose a base branch
Loading
fromFidget-Spinner:hoist_check_function

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-SpinnerFidget-Spinner commentedNov 15, 2024•
edited
Loading

Small PR to strength reduce _CHECK_FUNCTION so it reads from operand instead of reading from the frame struct. This prevents _CHECK_FUNCTION from interfering with the partial evaluation later on. It interferes because once we remove the frame, there is noframe->func_obj to check anymore.

An added benefit is that the "burned in" version is also cheaper as it doesn't have to read from the frame struct, rather reading from the instruction itself in the JIT.

@Fidget-Spinner
Copy link
MemberAuthor

@markshannon would you prefer this as its own PR, or would you prefer I merge this into the function inlining PR that I will be doing later?

@Fidget-Spinner
Copy link
MemberAuthor

Sorry I realized this is logically incorrect.

@Fidget-SpinnerFidget-Spinner changed the titlegh-120619: Hoist _CHECK_FUNCTION to frame sequence when possiblegh-120619: Strength reduce _CHECK_FUNCTIONNov 16, 2024
@markshannon
Copy link
Member

I think I've said this before, but please link to an appropriate issue.

Why are you doing this? The issue doesn't explain it at all.

@Fidget-SpinnerFidget-Spinner changed the titlegh-120619: Strength reduce _CHECK_FUNCTIONgh-126902: Strength reduce _CHECK_FUNCTIONNov 16, 2024
@Fidget-Spinner
Copy link
MemberAuthor

Fidget-Spinner commentedNov 16, 2024•
edited
Loading

@markshannon the issue is right, just too broad. I've created a smaller issue to further explain why we need this for partial evaluation at#126902

Copy link
Member

@markshannonmarkshannon left a comment

Choose a reason for hiding this comment

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

I think it would be better to use the symbol for the function, rather than a reference in the optimizer.

@@ -1515,9 +1515,33 @@ def test_jit_error_pops(self):
with self.assertRaises(TypeError):
{item for item in items}

def test_global_guard_strength_reduced_if_possible(self):
Copy link
Member

Choose a reason for hiding this comment

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

It would be nicer (not necessarily in this PR) if we created tracelets directly, rather than this complex approach.

@bedevere-app
Copy link

When you're done making the requested changes, leave the comment:I have made the requested changes; please review again.

And if you don't make the requested changes, you will be put in the comfy chair!

Copy link
Member

@markshannonmarkshannon left a comment

Choose a reason for hiding this comment

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

I'd like to replaceNULL withsym_new_unknown whenf_funcobj is not known.
Otherwise, LGTM.

@@ -4941,6 +4941,12 @@ dummy_func(
DEOPT_IF(func->func_version != func_version);
}

tier2 op(_CHECK_FUNCTION_UNMODIFIED, (func_version/2, callable_p/4 --)) {
assert(PyFunction_Check(callable_p));
PyFunctionObject *func = (PyFunctionObject *)callable_p;
Copy link
Member

@markshannonmarkshannonDec 6, 2024•
edited
Loading

Choose a reason for hiding this comment

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

What guarantee is there thatfunc == frame->f_funcobj?

Copy link
MemberAuthor

@Fidget-SpinnerFidget-SpinnerDec 8, 2024•
edited
Loading

Choose a reason for hiding this comment

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

It's constant propagated (and frames shouldn't be able to swap out f_funcobj on the fly like that). To be sure I added a runtime assert.

@python-cla-bot
Copy link

python-cla-botbot commentedApr 18, 2025•
edited
Loading

All commit authors signed the Contributor License Agreement.

CLA signed

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@markshannonmarkshannonmarkshannon requested changes

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@Fidget-Spinner@markshannon
[8]ページ先頭
©2009-2025 Movatter.jp