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

Fold unary + and not on constants #46751

Closed
abalkin opened this issue Mar 28, 2008 · 5 comments
Closed

Fold unary + and not on constants #46751

abalkin opened this issue Mar 28, 2008 · 5 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@abalkin
Copy link
Member

abalkin commented Mar 28, 2008

BPO 2499
Nosy @rhettinger, @abalkin, @pitrou
Files
  • fold-unary.diff: Patch against revision 61983
  • 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 = 'https://github.com/rhettinger'
    closed_at = <Date 2009-11-18.20:10:08.040>
    created_at = <Date 2008-03-28.04:05:17.936>
    labels = ['interpreter-core', 'performance']
    title = 'Fold unary + and not on constants'
    updated_at = <Date 2009-11-18.20:10:08.038>
    user = 'https://github.com/abalkin'

    bugs.python.org fields:

    activity = <Date 2009-11-18.20:10:08.038>
    actor = 'rhettinger'
    assignee = 'rhettinger'
    closed = True
    closed_date = <Date 2009-11-18.20:10:08.040>
    closer = 'rhettinger'
    components = ['Interpreter Core']
    creation = <Date 2008-03-28.04:05:17.936>
    creator = 'belopolsky'
    dependencies = []
    files = ['9880']
    hgrepos = []
    issue_num = 2499
    keywords = ['patch']
    message_count = 5.0
    messages = ['64613', '64618', '64638', '94905', '95446']
    nosy_count = 3.0
    nosy_names = ['rhettinger', 'belopolsky', 'pitrou']
    pr_nums = []
    priority = 'normal'
    resolution = 'out of date'
    stage = None
    status = 'closed'
    superseder = None
    type = 'resource usage'
    url = 'https://bugs.python.org/issue2499'
    versions = ['Python 2.6']

    @abalkin
    Copy link
    Member Author

    abalkin commented Mar 28, 2008

    Before:

    >>> dis(lambda:+2)
      1           0 LOAD_CONST               0 (2)
                  3 UNARY_POSITIVE      
                  4 RETURN_VALUE        
    >>> dis(lambda:not 2)
      1           0 LOAD_CONST               0 (2)
                  3 UNARY_NOT           
                  4 RETURN_VALUE        

    After:

    >>> dis(lambda:+2)
      1           0 LOAD_CONST               1 (2)
                  3 RETURN_VALUE        
    >>> dis(lambda:not 2)
      1           0 LOAD_CONST               1 (False)
                  3 RETURN_VALUE

    @abalkin abalkin added interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage labels Mar 28, 2008
    @rhettinger
    Copy link
    Contributor

    It would be helpful if we talked before going further on build-outs to
    the peephole optimizer. IIRC, we chose to not do this one because it
    interfered with other more important optimizations.

    More importantly, we decided that the peepholer is the wrong place to
    do much of this work. Most of the peepholer is going to be migrated
    up the chain, after the AST is generated, but before the opcodes are
    generated. That is a faster, more reliable, and more general
    approach.

    The constant folding anti-duplication patch should also not be done
    for this same reason. That patch slows down compilation and makes it
    more fragile but does not add speed (just like the dead code
    elimination patches). When the peepholer is moved-up, the anti-
    duplication code won't be needed (as you won't need its attendant
    rescan/rewrite pass of the bytecode).

    You're writing these faster than I have time to review and likely
    reject them. Please show some moderation. The peepholer is
    intentionally very conservative.

    @abalkin
    Copy link
    Member Author

    abalkin commented Mar 28, 2008

    On Fri, Mar 28, 2008 at 3:25 AM, Raymond Hettinger
    <report@bugs.python.org> wrote:

    It would be helpful if we talked before going further on build-outs to
    the peephole optimizer.

    Sure.

    IIRC, we chose to not do this one because it
    interfered with other more important optimizations.

    There are two optimization in my patch: one for + and the other
    not . I think you recall that not optimization could
    interfere with not a is b --> a is not b and similar transformations.
    My patch, however does not affect those. With respect to unary +, I
    don't think it was intentionally omitted: I would think it is more
    common to use unary + on constants than .., but case UNARY_CONVERT
    is there in 2.x.

    Constant folding promotes more readable code: 24*60*60 is more obvious
    than 86400, prefixing positive numbers with + leads to better visual
    alignment, etc. Users should not be required to think twice about
    which constant expressions are folded and which are not.

    Here is another surprise with the current peepholer:

    1 0 LOAD_CONST 0 (1)
    3 LOAD_CONST 3 (6)
    6 BINARY_ADD
    7 RETURN_VALUE

    1 0 LOAD_CONST 4 (7)
    3 RETURN_VALUE

    I have a fix in the works, but I will wait for your further comments
    before submitting it.

    More importantly, we decided that the peepholer is the wrong place to
    do much of this work. Most of the peepholer is going to be migrated
    up the chain, after the AST is generated, but before the opcodes are
    generated. That is a faster, more reliable, and more general
    approach.

    I agree. Constant folding, is an interesting case because peepholer
    has to duplicate a subset of eval logic. I wonder if the new approach
    could eliminate that.

    BTW, what is the rationale for leading +/- not being part of the
    number literal? Unary +/- optimization seems mostly useful for the
    simple +/-x case which could be handled by the tokenizer.

    What is the timeline for that project? Maybe a comment should be
    placed in peephole.c explaining that there is a plan to move
    uptimization logic up the compilation chain and announcing a
    moratorium on further peephole.c work. I am not the only one
    submitting peephole optimizer patches recently.

    You're writing these faster than I have time to review and likely
    reject them. Please show some moderation.

    ok :-) One more peephole optimizer related idea that I have in the
    pipeline is to implement an option to disable optimization altogether.
    Having such an option would help debugging/profiling the optimizer
    and will give users a simple check when they suspect that their code
    is improperly optimized. I would propose a patch already, but I
    could not think of a good command line option. Most compilers use
    -O0, but that would be confusingly similar to -OO. What do you think?

    @pitrou
    Copy link
    Member

    pitrou commented Nov 4, 2009

    Folding UNARY_POSITIVE was done by Raymond in r75601.

    @rhettinger rhettinger self-assigned this Nov 4, 2009
    @rhettinger
    Copy link
    Contributor

    Closing this. The Unary Positive is already implemented and there are
    no known use cases for constant folding a Unary Not.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants