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

Integer binary ops design discussion #743

Closed
TH3CHARLie opened this issue Jul 1, 2020 · 4 comments
Closed

Integer binary ops design discussion #743

TH3CHARLie opened this issue Jul 1, 2020 · 4 comments

Comments

@TH3CHARLie
Copy link
Collaborator

TH3CHARLie commented Jul 1, 2020

Start with a simple example:

def foo(left: int, right: int) -> bool:
     result = left == right
     return result

In the current C backend, it will generated C code(fully inlined except the out of memory handling):

#define CPY_INT_TAG 1

if (!(left & CPY_INT_TAG)) {
    result = left == right;
} else {
    if (!(right& CPY_INT_TAG)) {
        result = false;
    } else {
       result = PyObject_RichCompareBool((PyObject *)(left & ~CPY_INT_TAG),
                                              (PyObject *)(right & ~CPY_INT_TAG), Py_EQ);
        if (result == -1) {
            CPyError_OutOfMemory();
        }
    }
}
return result

So I guess the IR would look like

r0 = CPY_INT_TAG
r1 = BitwiseAnd(left, r0)
if not r1 goto L1 else goto L2

L1:
r2 = BinaryEq(left, right)
result = r2

L2:
r3 = BitwiseAnd(right, r0)
if not r2 goto L3 else goto L4

L3:
result = false

L4:
r4 = BitwiseNot(r0)
r5 = BitwiseAnd(left, r4)
r6 = BitwiseAnd(right, r4)
result = PyObject_RichCompareBool(r4, r5, PY_EQ) # we can have a custom CallC to do it
r7 = BinaryEq(result, -1)
if r7 goto L5 else goto L6

L5:
CPyError_OutOfMemory()

L6:
...

Instead of generating a single PrimitiveOp, we are trying to generate several blocks/branches. BitwiseNot, BitwiseAnd, BinaryEq should all be the binary ops we'd want to add.

cc @JukkaL

@TH3CHARLie TH3CHARLie changed the title Integer Binary Ops Design Discussion Integer binary ops design discussion Jul 1, 2020
@JukkaL
Copy link
Collaborator

JukkaL commented Jul 1, 2020

it will generated C code (fully inlined except the out of memory handling)

There's no need to inline the else block, since it's only used for long integers, and we can expect them to be relatively rare. Any performance benefit from inlining isn't worth the extra code size. So this would be a sufficient level of inlining:

#define CPY_INT_TAG 1

if (!(left & CPY_INT_TAG)) {
    result = left == right;
} else {
    result = CPyTagged_IsEq_(left, right);
}
return result

This would simplify the IR significantly.

Another thing is that we'd need an explicit comparison op after the bitwise and, for example:

  r0 = 1
  r1 = BitwiseAnd(left, r0)
  r2 = 0
  r3 = BinaryEq(r1, r2)
  if r3 goto L1 else goto L2
L1:
  r2 = BinaryEq(left, right)
  result = r2
  goto Lx
  ...

The generated IR is still a bit verbose. Maybe we should support integer literals as operands so we could represent the above IR more compactly like this:

  r1 = BitwiseAnd(left, 1)
  r2 = BinaryEq(r1, 0)
  if r2 goto L1 else goto L2
L1:
  r2 = BinaryEq(left, right)
  result = r2
  goto Lx
  ...

This might be worth it, since it looks like the low-level IR will have lots of low-level integer values. (This simplification can happen later.)

@TH3CHARLie
Copy link
Collaborator Author

Actually I find that we did register binary comparison ops for short ints(check int_ops.py -> int_compare_op for more info), but they never get called(I did the search through all mypyc IR tests, only the custom short int add unsafe_short_add is used)

@TH3CHARLie
Copy link
Collaborator Author

A good entry point would be transform_op_expr in expression.py. We can either use specializers or other helpers here. The high-level logic would be:

  1. check whether lhs and rhs are int
  2. build the above-discussed blocks if true
  3. fall back to original plan if false

@JukkaL
Copy link
Collaborator

JukkaL commented Jul 7, 2020

Integer ops are special enough that we can just implement them in a function called from transform_op_expr instead of using AST lookup driven specializers.

JukkaL pushed a commit to python/mypy that referenced this issue Jul 7, 2020
Related: mypyc/mypyc#741

This PR introduces BinaryIntOp to represent all low-level integer binary 
operations.

The block-like logic described in mypyc/mypyc#743 would be handled 
differently, BinaryIntOp would be the building block of it.
JukkaL pushed a commit to python/mypy that referenced this issue Jul 10, 2020
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

No branches or pull requests

2 participants