Skip to content

[mypyc] Split BinaryIntOp and introduce ComparisonOp, implements is/is not op#9313

Merged
TH3CHARLie merged 5 commits intopython:masterfrom
TH3CHARLie:comparison-op-refactor
Aug 18, 2020
Merged

[mypyc] Split BinaryIntOp and introduce ComparisonOp, implements is/is not op#9313
TH3CHARLie merged 5 commits intopython:masterfrom
TH3CHARLie:comparison-op-refactor

Conversation

@TH3CHARLie
Copy link
Copy Markdown
Collaborator

@TH3CHARLie TH3CHARLie commented Aug 15, 2020

BinaryIntOp used to represent arithmetic, bitwise and comparison operations on integer operations. However, this design prevents us to compare pointer types and all comparison operations should be of boolean return type while manually specifying this in BinaryIntOp is both verbose and error-prone.

This PR splits BinaryIntOp and moves the comparison functionalities to ComparsionOp.

Based on the new op, this PR also implements is and is not op.

@TH3CHARLie TH3CHARLie changed the title [mypyc] Split BinaryIntOp and introduce ComparisonOp [mypyc] Split BinaryIntOp and introduce ComparisonOp, implements is/is not op Aug 15, 2020
@TH3CHARLie TH3CHARLie requested a review from JukkaL August 15, 2020 05:06
@JukkaL
Copy link
Copy Markdown
Collaborator

JukkaL commented Aug 18, 2020

There are merge conflicts. Also, I can't see the definition of ComparisonOp in this PR. The first commit is somehow missing from this PR.

Copy link
Copy Markdown
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Left a few nits about a comment.

@TH3CHARLie
Copy link
Copy Markdown
Collaborator Author

TH3CHARLie commented Aug 18, 2020

@JukkaL Looks like the github review fail to represent to diff clearly, so here's the definition of two classes from 2764aa0:

class ComparisonOp(RegisterOp):
    """Comparison ops

    The result type will always be boolean.

    Support comparison between integer types and pointer types
    """
    error_kind = ERR_NEVER

    # S for signed and U for unsigned
    EQ = 100  # type: Final
    NEQ = 101  # type: Final
    SLT = 102  # type: Final
    SGT = 103  # type: Final
    SLE = 104  # type: Final
    SGE = 105  # type: Final
    ULT = 106  # type: Final
    UGT = 107  # type: Final
    ULE = 108  # type: Final
    UGE = 109  # type: Final

    op_str = {
        EQ: '==',
        NEQ: '!=',
        SLT: '<',
        SGT: '>',
        SLE: '<=',
        SGE: '>=',
        ULT: '<',
        UGT: '>',
        ULE: '<=',
        UGE: '>=',
    }  # type: Final

    def __init__(self, lhs: Value, rhs: Value, op: int, line: int = -1) -> None:
        super().__init__(line)
        self.type = bool_rprimitive
        self.lhs = lhs
        self.rhs = rhs
        self.op = op

    def sources(self) -> List[Value]:
        return [self.lhs, self.rhs]

    def to_str(self, env: Environment) -> str:
        if self.op in (self.SLT, self.SGT, self.SLE, self.SGE):
            sign_format = " :: signed"
        elif self.op in (self.ULT, self.UGT, self.ULE, self.UGE):
            sign_format = " :: unsigned"
        else:
            sign_format = ""
        return env.format('%r = %r %s %r%s', self, self.lhs,
                          self.op_str[self.op], self.rhs, sign_format)

    def accept(self, visitor: 'OpVisitor[T]') -> T:
        return visitor.visit_comparison_op(self)
class BinaryIntOp(RegisterOp):
    """Binary arithmetic and bitwise operations on integer types

    These ops are low-level and will be eventually generated to simple x op y form.
    The left and right values should be of low-level integer types that support those ops
    """
    error_kind = ERR_NEVER

    # arithmetic
    ADD = 0  # type: Final
    SUB = 1  # type: Final
    MUL = 2  # type: Final
    DIV = 3  # type: Final
    MOD = 4  # type: Final

    # bitwise
    AND = 200  # type: Final
    OR = 201  # type: Final
    XOR = 202  # type: Final
    LEFT_SHIFT = 203  # type: Final
    RIGHT_SHIFT = 204  # type: Final

    op_str = {
        ADD: '+',
        SUB: '-',
        MUL: '*',
        DIV: '/',
        MOD: '%',
        AND: '&',
        OR: '|',
        XOR: '^',
        LEFT_SHIFT: '<<',
        RIGHT_SHIFT: '>>',
    }  # type: Final

    def __init__(self, type: RType, lhs: Value, rhs: Value, op: int, line: int = -1) -> None:
        super().__init__(line)
        self.type = type
        self.lhs = lhs
        self.rhs = rhs
        self.op = op

    def sources(self) -> List[Value]:
        return [self.lhs, self.rhs]

    def to_str(self, env: Environment) -> str:
        return env.format('%r = %r %s %r', self, self.lhs,
                          self.op_str[self.op], self.rhs)

    def accept(self, visitor: 'OpVisitor[T]') -> T:
        return visitor.visit_binary_int_op(self)

@TH3CHARLie TH3CHARLie requested a review from JukkaL August 18, 2020 16:05
Copy link
Copy Markdown
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Looks good! The IR is now more consistent, as more low-level Python operations such as 'is' are represented using low-level IR ops. Feel free to merge after you've addressed my comment and the build is green.

"""cpy_r_r0 = PyDict_Contains(cpy_r_d, cpy_r_o);""")

def test_binary_int_op(self) -> None:
self.assert_emit(BinaryIntOp(short_int_rprimitive, self.s1, self.s2, BinaryIntOp.ADD, 1),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for adding more test coverage!

@TH3CHARLie TH3CHARLie merged commit a6ab096 into python:master Aug 18, 2020
@TH3CHARLie TH3CHARLie deleted the comparison-op-refactor branch August 18, 2020 17:21
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