Remove redundant macros used for stack manipulation in interpreter

[markshannon]

BPO	40925
Nosy	@rhettinger, @terryjreedy, @ericvsmith, @markshannon, @corona10, @sweeneyde, @shihai1991
PRs	bpo-40925: Remove unused stack macro SET_VALUE #20783 bpo-40925: Simplify stack manipulation macros #20845
Files	perf_diff.txt: Performance changes from PR 20845

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-06-13.13:10:03.011>
created_at = <Date 2020-06-09.10:13:40.788>
labels = ['interpreter-core', 'easy', 'performance']
title = 'Remove redundant macros used for stack manipulation in interpreter'
updated_at = <Date 2020-06-13.13:10:03.010>
user = 'https://github.com/markshannon'

bugs.python.org fields:

activity = <Date 2020-06-13.13:10:03.010>
actor = 'Mark.Shannon'
assignee = 'none'
closed = True
closed_date = <Date 2020-06-13.13:10:03.011>
closer = 'Mark.Shannon'
components = ['Interpreter Core']
creation = <Date 2020-06-09.10:13:40.788>
creator = 'Mark.Shannon'
dependencies = []
files = ['49230']
hgrepos = []
issue_num = 40925
keywords = ['patch', 'easy (C)']
message_count = 9.0
messages = ['371087', '371130', '371183', '371217', '371259', '371423', '371430', '371447', '371456']
nosy_count = 7.0
nosy_names = ['rhettinger', 'terry.reedy', 'eric.smith', 'Mark.Shannon', 'corona10', 'Dennis Sweeney', 'shihai1991']
pr_nums = ['20783', '20845']
priority = 'normal'
resolution = 'wont fix'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue40925'
versions = []

[markshannon]

Currently, there are fourteen macros provided for stack manipulation.
Along with the fundamental, POP(), PUSH() and PEEK(n) the following eleven are also provided:

TOP()          
SECOND()       
THIRD()        
FOURTH()       
SET_TOP(v)     
SET_SECOND(v)  
SET_THIRD(v)   
SET_FOURTH(v)  
SET_VALUE(n, v)
STACK_GROW(n)
STACK_SHRINK(n) 

These are unnecessary as compilers have long understood the sequences of pointer increments and decrements that is generated by using POPs and PUSHes.

See https://godbolt.org/z/Htw-2k which shows GCC generating exactly the same code from just POP, PUSH and PEEK as from direct access to items on the stack.

Removing the redundant macros would make the code easier to reason about and analyze.
TOP() and SECOND() are used quite heavily and are trivial to convert to PEEK by tools, so should probably be left alone.

Notes:

I'm ignoring the stack debugging macros here, they aren't a problem.
The EMPTY() macro is only used twice, so might as well be replaced with STACK_LEVEL == 0.
Sadly, there is no "maintainability/code quality" selection for "type" of issue, so I've chosen "performance" :(

[ericvsmith]

What type of changes are you proposing to make?

[markshannon]

I'm proposing that we remove the nine macros above; the eleven listed, except TOP() and SECOND().
They can be replaced by POP(), PUSH() and PEEK() without lost of functionality or performance.

[corona10]

AFAIK, SET_VALUE is not used in CPython master codebase.

[markshannon]

New changeset 33faf5c by Dong-hee Na in branch 'master':
bpo-40925: Remove unused stack macro SET_VALUE (GH-20783)
33faf5c

[sweeneyde]

I just added PR 20845, but I'm concerned about performance. I'm attaching the results of a run of pyperformance before and after PR 20845.

[rhettinger]

I don't see any advantage to making this change. The current code is readable and the macros have an obvious interpretation. We also know that they generate clean code on every compiler we've come across and on 32 bits.

Also, this should be marked as type, "performance". If performance changes at all, it will likely be a net detriment.

[terryjreedy]

I could understand wanting to replace TOP,...,FOURTH with PEEK(n) and the SET_XYZs with the now deleted SET(_VALUE)(n, v), but to me, replacing PEEK-SET with POPs and PUSHes (in different orders) that directly mean something different but that compilers (all?) optimize to mean the same thing makes the code initially harder to read. In particular, the old version of TARGET(ROT_FOUR) is initially clearer without thinking through the effect of the particular order of pushes in the new version. It depends on what one is accustomed to.

[markshannon]

Dennis, thanks for doing the benchmarking.
You seem to have removed rather more macros than I suggested,
but it probably won't make much difference to the results.

Since this may have a negative effect on performance and doesn't seem to be popular, let's drop it.

For the record, my motivation for this change is for tooling.
Instructions that only use POP, PEEK and PUSH, are much easier to analyze for tools like verifiers and optimizers.

Such "nice to have" features aren't worth the slowdown.