gh-131798: Split _CHECK_AND_ALLOCATE_OBJECT into smaller uops#148433
gh-131798: Split _CHECK_AND_ALLOCATE_OBJECT into smaller uops#148433Fidget-Spinner merged 7 commits intopython:mainfrom
_CHECK_AND_ALLOCATE_OBJECT into smaller uops#148433Conversation
|
Hey @Fidget-Spinner, I'm a little lost here
|
Python/optimizer_bytecodes.c
Outdated
| assert(init != NULL); | ||
| assert(PyFunction_Check(init)); | ||
| callable = sym_new_const(ctx, init); | ||
| sym_set_const(callable, init); |
There was a problem hiding this comment.
- This should go into the new _ALLOCATE_OBJECT
- It should be
callable = sym_new_const(ctx, init);notsym_set_const(callable, init);. There's a difference --- the first overrides the current callable with a new one, the second sets the current callable to a known constant. If you read the code in bytecodes.c, you'll see it's the first one.
Python/optimizer_bytecodes.c
Outdated
| PyType_Watch(TYPE_WATCHER_ID, (PyObject *)type); | ||
| _Py_BloomFilter_Add(dependencies, type); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same to bytecodes.c:
| } | |
| } | |
| self_or_null = sym_new_not_null(ctx); |
There was a problem hiding this comment.
Wait sorry, shouldn't this be sym_set_non_null?
There was a problem hiding this comment.
No. Please look at bytecodes.c. It's creating a new object, self. That's what _ALLOCATE_OBJECT does. So it should be a new non-null. This is same as the above where it grabs a new __init__ callable. In both of these cases, we do not set the existing symbol, but create a new one. This is a good learning point as it's tricky and requires observing bytecodes.c!
Co-authored-by: Hai Zhu <haiizhu@outlook.com>
|
@Sacul0457 I'm going to push to this PR, as I think it's a little tricky to get right. Please add a test after I push. |
|
@Sacul0457 please add the following tests:
|
|
Sorry for the wait, I printed the uops trace for the second test and how can we tell if it is constant folded? ['_START_EXECUTOR', '_MAKE_WARM', '_SET_IP', '_CHECK_PERIODIC', '_CHECK_VALIDITY', '_ITER_CHECK_RANGE', '_GUARD_NOT_EXHAUSTED_RANGE',
'_ITER_NEXT_RANGE', '_SET_IP', '_SWAP_FAST_1', '_SPILL_OR_RELOAD', '_POP_TOP', '_CHECK_VALIDITY', '_SET_IP', '_LOAD_DEREF',
'_CHECK_VALIDITY', '_PUSH_NULL', '_LOAD_CONST_INLINE_BORROW', '_SPILL_OR_RELOAD', '_LOAD_CONST_INLINE_BORROW', '_SET_IP',
'_SPILL_OR_RELOAD', '_CHECK_OBJECT', '_ALLOCATE_OBJECT', '_CREATE_INIT_FRAME', '_PUSH_FRAME', '_CHECK_VALIDITY',
'_TIER2_RESUME_CHECK', '_LOAD_CONST_INLINE_BORROW', '_SET_IP', '_RETURN_VALUE', '_CHECK_VALIDITY', '_SET_IP', '_EXIT_INIT_CHECK',
'_CHECK_VALIDITY', '_SET_IP', '_MAKE_HEAP_SAFE', '_RETURN_VALUE', '_GUARD_IP_RETURN_VALUE', '_GUARD_CODE_VERSION_RETURN_VALUE',
'_CHECK_VALIDITY', '_SET_IP', '_SWAP_FAST_2', '_POP_TOP', '_CHECK_VALIDITY', '_SET_IP', '_LOAD_DEREF', '_CHECK_VALIDITY',
'_PUSH_NULL', '_LOAD_CONST_INLINE_BORROW', '_SPILL_OR_RELOAD', '_LOAD_CONST_INLINE_BORROW', '_SET_IP', '_SPILL_OR_RELOAD',
'_CHECK_OBJECT', '_ALLOCATE_OBJECT', '_CREATE_INIT_FRAME', '_PUSH_FRAME', '_CHECK_VALIDITY', '_TIER2_RESUME_CHECK',
'_LOAD_CONST_INLINE_BORROW', '_SET_IP', '_RETURN_VALUE', '_CHECK_VALIDITY', '_SET_IP', '_EXIT_INIT_CHECK', '_CHECK_VALIDITY',
'_SET_IP', '_MAKE_HEAP_SAFE', '_RETURN_VALUE', '_GUARD_IP_RETURN_VALUE', '_GUARD_CODE_VERSION_RETURN_VALUE', '_CHECK_VALIDITY', '_SET_IP', '_SWAP_FAST_2', '_POP_TOP', '_JUMP_TO_TOP', ...] |
|
@Sacul0457 that's strange, seems like it's not being constant folded in the second case. Let me fix that. |
|
Oh alright. I'm probably going to bed now, please feel free to add the tests too as I don't want to hold up this PR being merged. Thank you! |
|
Sorry Sacul, I forgot to mention that you can't nest |
Fidget-Spinner
left a comment
There was a problem hiding this comment.
Thank you for splitting the ops in bytecodes.c Sacul.
Uh oh!
There was an error while loading. Please reload this page.