GH-128939: Refactor JIT optimize structs #128940
Conversation
|
Can you add a test that the tuple information is actually propagated? Either through testing type guard elimination happens or something. Unless you plan to refactor the tests as well :). |
|
Random comment: JIT is an acronym, and we use the "JIT" capitalization elsewhere. "Jit" just looks... wrong. |
There was a problem hiding this comment.
Various thoughts and questions I had while reading through. I think this is going to be a very nice improvement, though!
Other thoughts:
- It's probably worth adding a helper for immortality checks. Basically, check if we have an immortal constant, or a known
booltype. The bool part is nice, but easy to forget. - Any plans to update the
_Py_uop_naming to be "JIT"-ier?
| typedef union _jit_opt_symbol { | ||
| uint8_t tag; | ||
| JitOptKnownClass cls; | ||
| JitOptKnownValue value; | ||
| JitOptKnownVersion version; | ||
| JitOptTuple tuple; | ||
| } JitOptSymbol; |
There was a problem hiding this comment.
Could this be:
| typedef union _jit_opt_symbol { | |
| uint8_t tag; | |
| JitOptKnownClass cls; | |
| JitOptKnownValue value; | |
| JitOptKnownVersion version; | |
| JitOptTuple tuple; | |
| } JitOptSymbol; | |
| typedef struct { | |
| uint8_t tag; | |
| union { | |
| JitOptKnownClass cls; | |
| JitOptKnownValue value; | |
| JitOptKnownVersion version; | |
| JitOptTuple tuple; | |
| }; | |
| } JitOptSymbol; |
Then we wouldn't need to repeat the tag in each struct.
Or does the current scheme potentially pack better?
| typedef struct _jit_opt_known_class { | ||
| uint8_t tag; | ||
| uint32_t version; | ||
| PyTypeObject *type; | ||
| } JitOptKnownClass; |
There was a problem hiding this comment.
We could probably just get the version by doing cls.type->tp_version_tag, right? Having both here just creates opportunities for them to be stale or out-of-sync, I think.
| typedef struct _jit_opt_tuple { | ||
| uint8_t tag; | ||
| uint8_t length; | ||
| uint16_t items[6]; |
There was a problem hiding this comment.
I think we have room for 7, right?
| uint16_t items[6]; | |
| uint16_t items[7]; |
| case JIT_SYM_KNOWN_VALUE_TAG: | ||
| Py_CLEAR(sym->value.value); | ||
| sym_set_bottom(ctx, sym); | ||
| return false; | ||
| case JIT_SYM_TUPLE_TAG: | ||
| sym_set_bottom(ctx, sym); | ||
| return false; |
There was a problem hiding this comment.
These could potentially have the same version tag, right? Looking up a method or whatever on a constant value is a normal thing to do.
| case JIT_SYM_TUPLE_TAG: | ||
| sym_set_bottom(ctx, sym); | ||
| return; |
There was a problem hiding this comment.
This also seems like it could potentially be fine (transitioning from an abstract tuple to a concrete one)... could potentially need to do something recursive for the elements, though.
| _Py_UopsSymbol * | ||
| _Py_uop_sym_new_unknown(_Py_UOpsContext *ctx) | ||
| JitOptSymbol * | ||
| _Py_uop_sym_new_unknown(JitOptContext *ctx) | ||
| { | ||
| return sym_new(ctx); |
There was a problem hiding this comment.
Should this be checked for NULL and replaced with out_of_space like the others?
| _Py_uop_sym_tuple_getitem(JitOptContext *ctx, JitOptSymbol *sym, int item) | ||
| { | ||
|
|
||
| if (sym->tag != JIT_SYM_TUPLE_TAG || item < 0 || item >= sym->tuple.length) { |
There was a problem hiding this comment.
We can handle negative indexes pretty easily here, right?
Also, this can probably be updated to work correctly for constant tuples.
| int | ||
| _Py_uop_sym_tuple_length(JitOptSymbol *sym) | ||
| { | ||
| if (sym->tag != JIT_SYM_TUPLE_TAG) { |
There was a problem hiding this comment.
Same here, this can probably be updated to work for constants.
| if (size > 6) { | ||
| res->tag = JIT_SYM_KNOWN_CLASS_TAG; | ||
| res->cls.type = &PyTuple_Type; | ||
| } |
There was a problem hiding this comment.
There are probably cases where this can be represented as a constant value (when all items are known).
There was a problem hiding this comment.
Hm, although it could get tricky because nothing owns that constant, so we have to be careful not to promote its uses to constant loads.
Converts the ad-hoc set of flags in
_Py_UopsSymbolinto a tagged union.This:
enumin a switch allows the C compiler to tell us if we miss any cases_Py_Uopsto start withJitOpt.