gh-148380: remove all uses of _PyType_LookupByVersion in optimizer_bytecodes.c

[NekoAsakura]

I couldn't add _RECORD_TOS to LOAD_ATTR_CLASS_WITH_METACLASS_CHECK since the recording slot's already taken by _RECORD_TOS_TYPE.

cpython/Tools/cases_generator/analyzer.py

Lines 1135 to 1150 in e36f8db

	first = True
	for part in macro.uops:
	match part:
	case parser.OpName():
	if part.name == "flush":
	parts.append(Flush())
	else:
	if part.name not in uops:
	raise analysis_error(
	f"No Uop named {part.name}", macro.tokens[0]
	)
	uop = uops[part.name]
	if uop.properties.records_value and not first:
	raise analysis_error(
	f"Recording uop {part.name} must be first in macro",
	macro.tokens[0])

So there's a minor optimisation regression on the metaclass path, but not much we can do about it.
As for naming, it's a right mix in the file, so I just went with what felt consistent.

• Issue: JIT: remove all uses of _PyType_LookupByVersion in optimizer_bytecodes.c #148380

[Fidget-Spinner]

So there's a minor optimisation regression on the metaclass path,

Oh dear, thanks for pointing that out. That's the one for enum.attr optimization, so we can't really make that worse.

I suggest we wait for #148378 to be done and then do that?

[NekoAsakura]

Right, hold off then. Give me a kick once #148378 lands :)

[Fidget-Spinner]

Btw, It think this is bugged on main, there needs to be a watcher installed on the type for this path similar to _GUARD_TYPE_VERSION. would you like to open a PR to fix it?

[NekoAsakura]

Sure! Which issue should I link to?

[Fidget-Spinner]

Thank you. I think it's fine to open a new issue describing the bug.

[Fidget-Spinner]

The other PR is merged. I think you're already in the mist of changing _GUARD_TYPE_VERSION in #148528. So it's just _CHECK_ATTR_CLASS for this PR then.

[Fidget-Spinner]

I couldn't add _RECORD_TOS to LOAD_ATTR_CLASS_WITH_METACLASS_CHECK since the recording slot's already taken by _RECORD_TOS_TYPE.

Sorry, but why can't we replace RECORD_TOS_TYPE with RECORD_TOS? RECORD_TOS records more information than RECORD_TOS_TYPE after all.

[NekoAsakura]

Sorry, but why can't we replace RECORD_TOS_TYPE with RECORD_TOS? RECORD_TOS records more information than RECORD_TOS_TYPE after all.

Fair, I didn't check at the time whether _RECORD_TOS_TYPE is a strict subset of _RECORD_TOS, but looking now it is. I will replace it.

[Fidget-Spinner]

Thanks

[Fidget-Spinner]

This needs to be changed to

assert(this_instr[-1].opcode == _RECORD_TOS_TYPE || this_instr[-1].opcode == _RECORD_TOS);

[Fidget-Spinner]

Hmm I suspect the new thing we're bumping up against is #148439

Unfortunately, I do not know if that's actually the root cause or hiding another bug.

[NekoAsakura]

I am investigating.

[NekoAsakura]

I think it is a bug in deopts. Let me draft a issue.