Skip to content

gh-104909: Implement conditional stack effects for macros #105748

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

Merged
merged 14 commits into from
Jun 14, 2023
Merged
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 43 additions & 14 deletions Tools/cases_generator/generate_cases.py
Original file line number Diff line number Diff line change
Expand Up @@ -191,14 +191,20 @@ def stack_adjust(
if osym and osym != isym:
self.emit(f"STACK_GROW({osym});")

def declare(self, dst: StackEffect, src: StackEffect | None):
def declare(
self, dst: StackEffect,
src: StackEffect | None,
*,
# Don't initialize from dst.cond; used for conditional *outputs*.
unconditional: bool = False,
Copy link
Member

Choose a reason for hiding this comment

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

I'm finding it hard to track the directions of the different booleans here (unconditional is used for conditional outputs, and we don't initialise when it is True..).

Copy link
Member Author

Choose a reason for hiding this comment

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

That's fair. I'm reverting the sense and renaming it to init_null, with a default of True (since that's what's needed in 5 out of 6 call sites).

):
if dst.name == UNUSED:
return
typ = f"{dst.type}" if dst.type else "PyObject *"
if src:
cast = self.cast(dst, src)
init = f" = {cast}{src.name}"
elif dst.cond:
elif dst.cond and not unconditional:
init = " = NULL"
else:
init = ""
Expand Down Expand Up @@ -491,7 +497,7 @@ class OverriddenInstructionPlaceHolder:
name: str


AnyInstruction = Instruction | MacroInstruction
AnyInstruction = Instruction | MacroInstruction | PseudoInstruction
INSTR_FMT_PREFIX = "INSTR_FMT_"


Expand Down Expand Up @@ -530,6 +536,7 @@ def error(self, msg: str, node: parser.Node) -> None:
macros: dict[str, parser.Macro]
macro_instrs: dict[str, MacroInstruction]
families: dict[str, parser.Family]
pseudos: dict[str, parser.Pseudo]
pseudo_instrs: dict[str, PseudoInstruction]

def parse(self) -> None:
Expand Down Expand Up @@ -587,7 +594,7 @@ def parse_file(self, filename: str, instrs_idx: dict[str, int]) -> None:

# Parse from start
psr.setpos(start)
thing: parser.InstDef | parser.Macro | parser.Family | None
thing: parser.InstDef | parser.Macro | parser.Pseudo | parser.Family | None
thing_first_token = psr.peek()
while thing := psr.definition():
match thing:
Expand Down Expand Up @@ -820,9 +827,14 @@ def stack_analysis(

Ignore cache effects.

Return the list of variable names and the initial stack pointer.
Return the list of variables (as StackEffects) and the initial stack pointer.
"""
lowest = current = highest = 0
conditions: dict[int, str] = {} # Indexed by 'current'.
last_instr: Instruction | None = None
for thing in components:
if isinstance(thing, Instruction):
last_instr = thing
for thing in components:
match thing:
case Instruction() as instr:
Expand All @@ -835,9 +847,24 @@ def stack_analysis(
"which are not supported in macro instructions",
instr.inst, # TODO: Pass name+location of macro
)
if any(eff.cond for eff in instr.input_effects):
self.error(
f"Instruction {instr.name!r} has conditional input stack effect, "
"which are not supported in macro instructions",
instr.inst, # TODO: Pass name+location of macro
)
if any(eff.cond for eff in instr.output_effects) and instr is not last_instr:
self.error(
f"Instruction {instr.name!r} has conditional output stack effect, "
"but is not the last instruction in a macro",
instr.inst, # TODO: Pass name+location of macro
)
current -= len(instr.input_effects)
lowest = min(lowest, current)
current += len(instr.output_effects)
for eff in instr.output_effects:
if eff.cond:
conditions[current] = eff.cond
current += 1
highest = max(highest, current)
case parser.CacheEffect():
pass
Expand All @@ -846,9 +873,9 @@ def stack_analysis(
# At this point, 'current' is the net stack effect,
# and 'lowest' and 'highest' are the extremes.
# Note that 'lowest' may be negative.
# TODO: Reverse the numbering.
stack = [
StackEffect(f"_tmp_{i+1}", "") for i in reversed(range(highest - lowest))
StackEffect(f"_tmp_{i}", "", conditions.get(highest - i, ""))
for i in reversed(range(1, highest - lowest + 1))
]
return stack, -lowest

Expand Down Expand Up @@ -880,15 +907,17 @@ def effect_str(effects: list[StackEffect]) -> str:
low = 0
sp = 0
high = 0
pushed_symbolic: list[str] = []
for comp in parts:
for effect in comp.instr.input_effects:
assert not effect.cond, effect
assert not effect.size, effect
sp -= 1
low = min(low, sp)
for effect in comp.instr.output_effects:
assert not effect.cond, effect
assert not effect.size, effect
if effect.cond:
pushed_symbolic.append(maybe_parenthesize(f"{maybe_parenthesize(effect.cond)} ? 1 : 0"))
sp += 1
high = max(sp, high)
if high != max(0, sp):
Expand All @@ -898,9 +927,10 @@ def effect_str(effects: list[StackEffect]) -> str:
# calculations to use the micro ops.
self.error("Macro has virtual stack growth", thing)
popped = str(-low)
pushed = str(sp - low)
pushed_symbolic.append(str(sp - low - len(pushed_symbolic)))
pushed = " + ".join(pushed_symbolic)
case parser.Pseudo():
instr = self.pseudos[thing.name]
instr = self.pseudo_instrs[thing.name]
popped = pushed = None
# Calculate stack effect, and check that it's the the same
# for all targets.
Expand Down Expand Up @@ -1168,12 +1198,11 @@ def wrap_macro(self, mac: MacroInstruction):
src = None
if i < mac.initial_sp:
src = StackEffect(f"stack_pointer[-{mac.initial_sp - i}]", "")
self.out.declare(var, src)
self.out.declare(var, src, unconditional=True)

yield

# TODO: Use slices of mac.stack instead of numeric values
self.out.stack_adjust(mac.final_sp - mac.initial_sp, [], [])
self.out.stack_adjust(0, mac.stack[:mac.initial_sp], mac.stack[:mac.final_sp])

for i, var in enumerate(reversed(mac.stack[: mac.final_sp]), 1):
dst = StackEffect(f"stack_pointer[-{i}]", "")
Expand Down