gh-104635: Add NO_EXCEPTION flag to opcode metadata#106394
gh-104635: Add NO_EXCEPTION flag to opcode metadata#106394corona10 wants to merge 3 commits intopython:mainfrom
Conversation
4d2ac67 to
4025d49
Compare
4025d49 to
47b2995
Compare
|
What is the heuristic to determine whether an opcode raises? |
I thought that we should use a blocklist approach or allowlist approach for classifying opcodes.
The reason for skipping |
carljm
left a comment
There was a problem hiding this comment.
It's not clear to me that "exception-raising" status of an opcode changes enough that we need to automatically determine this metadata via heuristic on the opcode implementation, rather than just manually maintain it.
I'm not necessarily opposed, if we can find a sufficiently conservative heuristic that does not fail in the unsafe direction. But I'm not sure I see any such possible heuristic, given my inline comment.
| HAS_CONST_FLAG: bool | ||
| HAS_NAME_FLAG: bool | ||
| HAS_JUMP_FLAG: bool | ||
| NO_EXCEPTION_FLAG: bool |
There was a problem hiding this comment.
Negative flags tend to lead to confusing double-negative code constructs. So I would prefer for the flag to be CAN_RAISE_FLAG than NO_EXCEPTION_FLAG, and similar throughout the PR.
| if token.kind == "IDENTIFIER": | ||
| if token_text == "error_if": | ||
| return False | ||
| if token_text.startswith("py") or token_text.startswith("_py"): |
There was a problem hiding this comment.
There are static helper functions in ceval.c that are used by opcodes and not Py prefixed, and some of these can raise. E.g. match_class. We can special case the existing ones, but someone could add a new one at any time. So I don't think this heuristic is safe, and I'm not sure I see any possible heuristic that would be safe in the face of possible future changes.
There was a problem hiding this comment.
I guess the match_class case should be covered by ERROR_IF used afterward to check if _PyErr_Occurred. But what if someone calls a non Py prefixed helper function and then uses a manual goto error; rather than ERROR_IF? There are some opcodes that use goto error; directly...
Maybe checking for both ERROR_IF and goto error would be adequate, and we wouldn't even need to check for use of Py prefixed API? For an opcode implementation to handle an error correctly, it seems it needs to goto error or ERROR_IF. Unless someone introduces another new macro that includes goto error, that would break the heuristic.
Existing macros that implicitly goto error include CHECK_EVAL_BREAKER, DECREF_INPUTS_AND_REUSE_FLOAT, and INSTRUMENTED_JUMP.
This PR is motivated from comments from @iritkatriel and @carljm.
But I'm not sure if this is the concept Irit originally intended, so I write the PR as the PoC with heuristic codes that will not raise exceptions.
If there is something I missed, please let me know.
We can use this metadata for aggressive optimization between two opcodes that will not raise exceptions.