Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Partially implemented type propagation #48

Merged
merged 16 commits into from
Oct 12, 2023
Merged

Conversation

JuliaPoo
Copy link
Collaborator

This branch will be continually updated until it is convenient to merge with ssa_graphs

  1. implements the types for the symbolics relevant to the current guards
  2. modifies the dsl and bytecodes.c to encode the type prop rules
  3. changes abstract interpreter for impure ops to only track stack pointer changes
  4. added type initialisation of symbolics from constants (sym_const_init)
  5. added type propagation for pure operations

// bitmask of types
uint32_t types;
// auxillary data for the types
uint32_t aux[MAX_TYPE + 1];
Copy link
Owner

Choose a reason for hiding this comment

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

Is this due to the fact that a single thing could be of multiple positive types?

Would it be possible to not have this, and only have something identify as a single positive type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, its bcuz multiple guards has to pass for some cases inorder to fully eliminate the entire guard region. I dont see whats the disadvantage of doing this either

Copy link
Owner

Choose a reason for hiding this comment

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

Every time we add a new type, all sym expressions become 32 bits bigger. I would like to save memory somehow. But ok lets go with this for now.

Copy link
Owner

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

Just one code nit from me, thank you.

static void
symtype_set_from_const(_Py_UOpsSymType* sym_type, PyObject* obj)
{
PyTypeObject *tp = obj->ob_type;
Copy link
Owner

Choose a reason for hiding this comment

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

Usually these are accessed through macros

Suggested change
PyTypeObject *tp = obj->ob_type;
PyTypeObject *tp = Py_TYPE(obj);

Copy link
Owner

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

Also bunch of whitespaces are failing the CI


/*
static _Py_UOpsSymExprTypeEnum
symtype_guard_to_type_enum(int guard_opcde)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
symtype_guard_to_type_enum(int guard_opcde)
symtype_guard_to_type_enum(int guard_opcde)

}

static bool
symtype_passes_guard(_Py_UOpsSymType* sym_type, int guard_opcode, uint32_t aux)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
symtype_passes_guard(_Py_UOpsSymType* sym_type, int guard_opcode, uint32_t aux)
symtype_passes_guard(_Py_UOpsSymType* sym_type, int guard_opcode, uint32_t aux)

*/

static void
symtype_set_type(_Py_UOpsSymType* sym_type, _Py_UOpsSymExprTypeEnum typ, uint32_t aux)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
symtype_set_type(_Py_UOpsSymType* sym_type, _Py_UOpsSymExprTypeEnum typ, uint32_t aux)
symtype_set_type(_Py_UOpsSymType* sym_type, _Py_UOpsSymExprTypeEnum typ, uint32_t aux)


if (tp == &PyLong_Type) {
sym_type->types |= 1 << PYINT_TYPE;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
}
}

if (_PyDictOrValues_IsValues(*dorv) ||
_PyObject_MakeInstanceAttributesFromDict(obj, dorv)) {
sym_type->types |= 1 << GUARD_DORV_VALUES_INST_ATTR_FROM_DICT_TYPE;

Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change

Comment on lines 189 to 190
self.peeks.append(
StackItem(offset=self.final_offset.clone(), effect=eff))
Copy link
Owner

Choose a reason for hiding this comment

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

Lets reduce the diff

Suggested change
self.peeks.append(
StackItem(offset=self.final_offset.clone(), effect=eff))
self.peeks.append(StackItem(offset=self.final_offset.clone(), effect=eff))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah I didn't realise but at some point during the buggy vscode fiasco it ran the autoformatter

Comment on lines 193 to 194
self.pokes.append(
StackItem(offset=self.final_offset.clone(), effect=eff))
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
self.pokes.append(
StackItem(offset=self.final_offset.clone(), effect=eff))
self.pokes.append(StackItem(offset=self.final_offset.clone(), effect=eff))

@@ -262,14 +270,24 @@ def cache_effect(self) -> CacheEffect | None:

@contextual
def stack_effect(self) -> StackEffect | None:
# IDENTIFIER [':' IDENTIFIER [TIMES]] ['if' '(' expression ')']
# IDENTIFIER [':' [IDENTIFIER [TIMES]] ['~' '(' IDENTIFIER [',' IDENTIFIER] ')']] ['if' '(' expression ')']
Copy link
Owner

Choose a reason for hiding this comment

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

The MINUS isn't documented.

@Fidget-Spinner
Copy link
Owner

Thanks for all your work on this.

Copy link
Owner

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

PEP 7 nits

} _Py_UOpsSymType;

static void
symtype_set_type(_Py_UOpsSymType* sym_type, _Py_UOpsSymExprTypeEnum typ, uint32_t aux)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
symtype_set_type(_Py_UOpsSymType* sym_type, _Py_UOpsSymExprTypeEnum typ, uint32_t aux)
symtype_set_type(_Py_UOpsSymType *sym_type, _Py_UOpsSymExprTypeEnum typ, uint32_t aux)

}

static void
symtype_set_from_const(_Py_UOpsSymType* sym_type, PyObject* obj)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
symtype_set_from_const(_Py_UOpsSymType* sym_type, PyObject* obj)
symtype_set_from_const(_Py_UOpsSymType *sym_type, PyObject *obj)

# Conflicts:
#	Python/abstract_interp_cases.c.h
#	Python/bytecodes.c
#	Tools/cases_generator/stacking.py
@Fidget-Spinner Fidget-Spinner merged commit 161c271 into ssa_graphs Oct 12, 2023
5 of 7 checks passed
@Fidget-Spinner Fidget-Spinner deleted the ssa_graphs_jules branch October 12, 2023 14:46
@JuliaPoo JuliaPoo restored the ssa_graphs_jules branch October 12, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants