| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| Expand Up | @@ -261,15 +261,19 @@ def adjust_higher(self, eff: StackEffect) -> None: | |
| self.final_offset.higher(eff) | ||
|
|
||
| def adjust(self, offset: StackOffset) -> None: | ||
| for down in offset.deep: | ||
| deep = list(offset.deep) | ||
| high = list(offset.high) | ||
| for down in deep: | ||
| self.adjust_deeper(down) | ||
| for up in offset.high: | ||
| for up in high: | ||
| self.adjust_higher(up) | ||
|
|
||
| def adjust_inverse(self, offset: StackOffset) -> None: | ||
| for down in offset.deep: | ||
| deep = list(offset.deep) | ||
| high = list(offset.high) | ||
| for down in deep: | ||
| self.adjust_higher(down) | ||
| for up in offset.high: | ||
| for up in high: | ||
| self.adjust_deeper(up) | ||
|
|
||
| def collect_vars(self) -> dict[str, StackEffect]: | ||
| Expand Down Expand Up | @@ -426,15 +430,26 @@ def write_components( | |
| ) | ||
|
|
||
| if mgr.instr.name in ("_PUSH_FRAME", "_POP_FRAME"): | ||
| # Adjust stack to min_offset (input effects materialized) | ||
| # Adjust stack to min_offset. | ||
| # This means that all input effects of this instruction | ||
| # are materialized, but not its output effects. | ||
| # That's as intended, since these two are so special. | ||
| out.stack_adjust(mgr.min_offset.deep, mgr.min_offset.high) | ||
| # Use clone() since adjust_inverse() mutates final_offset | ||
| mgr.adjust_inverse(mgr.final_offset.clone()) | ||
| # However, for tier 2, pretend the stack is at final offset. | ||
| mgr.adjust_inverse(mgr.final_offset) | ||
| if tier == TIER_ONE: | ||
| # TODO: Check in analyzer that _{PUSH,POP}_FRAME is last. | ||
| assert ( | ||
| mgr is managers[-1] | ||
| ), f"Expected {mgr.instr.name!r} to be the last uop" | ||
| assert_no_pokes(managers) | ||
|
|
||
| if mgr.instr.name == "SAVE_CURRENT_IP": | ||
| next_instr_is_set = True | ||
| if cache_offset: | ||
| out.emit(f"next_instr += {cache_offset};") | ||
| if tier == TIER_ONE: | ||
| assert_no_pokes(managers) | ||
|
Comment thread
Comment on lines
+445
to
+452
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityWhy are pokes bad for these?
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityBecause I don't want to think about what to do with loose pokes in macros containing SAVE_CURRENT_IP (i.e. some specializations of CALL), since next_instr_is_set causes the normal handling of loose pokes below to be skipped. If and when we need to support that case, this assert will remind us (or at least me :-) that some additional effort is needed here. (Kind of like how the LOAD_FAST_PUSH_NULL use case reminded me to fix this issue here.)
Sorry, something went wrong.
brandtbucher reacted with thumbs up emoji
All reactions
|
||
|
|
||
| if len(parts) == 1: | ||
| mgr.instr.write_body(out, 0, mgr.active_caches, tier) | ||
| Expand All | @@ -443,19 +458,41 @@ def write_components( | |
| mgr.instr.write_body(out, -4, mgr.active_caches, tier) | ||
|
|
||
| if mgr is managers[-1] and not next_instr_is_set: | ||
| # TODO: Explain why this adjustment is needed. | ||
| # Adjust the stack to its final depth, *then* write the | ||
| # pokes for all preceding uops. | ||
| # Note that for array output effects we may still write | ||
| # past the stack top. | ||
|
Comment thread
Comment on lines
+463
to
+464
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThis seems surprising to me. When could this happen?
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityE.g. in UNPACK_SEQUENCE and its specializations. For array outputs, the variable is just a pointer, and it is initialized before the body of the opcode is executed. The body is supposed to write its output directly into the stack using this pointer. But the SP is not adjusted until after the body is executed. This is iffy only if you believe that something else might write over the portion of the stack above the SP while that code is running. But nothing ever does that.
Sorry, something went wrong.
brandtbucher reacted with thumbs up emoji
All reactions
|
||
| out.stack_adjust(mgr.final_offset.deep, mgr.final_offset.high) | ||
| # Use clone() since adjust_inverse() mutates final_offset | ||
| mgr.adjust_inverse(mgr.final_offset.clone()) | ||
| write_all_pokes(mgr.final_offset, managers, out) | ||
|
|
||
| return next_instr_is_set | ||
|
|
||
|
|
||
| def assert_no_pokes(managers: list[EffectManager]) -> None: | ||
| for mgr in managers: | ||
| for poke in mgr.pokes: | ||
| if not poke.effect.size and poke.effect.name not in mgr.instr.unmoved_names: | ||
| out.assign( | ||
| poke.as_stack_effect(), | ||
| poke.effect, | ||
| ) | ||
| assert ( | ||
| poke.effect.name == UNUSED | ||
| ), f"Unexpected poke of {poke.effect.name} in {mgr.instr.name!r}" | ||
|
|
||
| return next_instr_is_set | ||
|
|
||
| def write_all_pokes( | ||
| offset: StackOffset, managers: list[EffectManager], out: Formatter | ||
| ) -> None: | ||
| # Emit all remaining pushes (pokes) | ||
| for m in managers: | ||
| m.adjust_inverse(offset) | ||
| write_pokes(m, out) | ||
|
|
||
|
|
||
| def write_pokes(mgr: EffectManager, out: Formatter) -> None: | ||
| for poke in mgr.pokes: | ||
| if not poke.effect.size and poke.effect.name not in mgr.instr.unmoved_names: | ||
| out.assign( | ||
| poke.as_stack_effect(), | ||
| poke.effect, | ||
| ) | ||
|
|
||
|
|
||
| def write_single_instr_for_abstract_interp(instr: Instruction, out: Formatter) -> None: | ||
| Expand All | @@ -478,8 +515,7 @@ def _write_components_for_abstract_interp( | |
| for mgr in managers: | ||
| if mgr is managers[-1]: | ||
| out.stack_adjust(mgr.final_offset.deep, mgr.final_offset.high) | ||
| # Use clone() since adjust_inverse() mutates final_offset | ||
| mgr.adjust_inverse(mgr.final_offset.clone()) | ||
| mgr.adjust_inverse(mgr.final_offset) | ||
| # NULL out the output stack effects | ||
| for poke in mgr.pokes: | ||
| if not poke.effect.size and poke.effect.name not in mgr.instr.unmoved_names: | ||
| Expand Down | ||
| Back | FazBrowse Home | New Git URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityJust curious, why? I assume it has something to do with the comment you removed below (# Use clone() since adjust_inverse() mutates final_offset), but I'm not sure I understand.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityIndeed. That .clone() call was needed because adjust_higher() and adjust_deeper() change self.final_offset (in place), and the argument here may be self.final_offset. (And ditto for min_offset.) So previously my "fix" for this was to pass in a clone of the argument. The new fix avoids the need to clone in the caller but effectively clones the deep and high lists of the argument.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.