[mypyc] new error_kind and branch variant to handle call_negative_bool_emit

[TH3CHARLie]

related mypyc/mypyc#734 and mypyc/mypyc#741.

Introducing ERR_NEG_INT error_kind and NEG_INT_EXPR branch variant to support checking the return value of a call is non-negative. set.discard is used as an example. With some modifications, this would also support negative_int_emit.

[TH3CHARLie]

Generated code:

before:

    PyObject *cpy_r_r0;
    char cpy_r_r1;
    char cpy_r_r2;
    PyObject *cpy_r_r3;
CPyL0: ;
    CPyTagged_IncRef(cpy_r_y);
    cpy_r_r0 = CPyTagged_StealAsObject(cpy_r_y);
    cpy_r_r1 = PySet_Discard(cpy_r_x, cpy_r_r0) >= 0;
    CPy_DecRef(cpy_r_r0);
    if (unlikely(!cpy_r_r1)) {
        CPy_AddTraceback("foo.py", "fossss", 4, CPyStatic_globals);
        goto CPyL2;
    } else
        goto CPyL1;

after:

    PyObject *cpy_r_r0;
    Py_ssize_t cpy_r_r1;
    char cpy_r_r2;
    PyObject *cpy_r_r3;
CPyL0: ;
    CPyTagged_IncRef(cpy_r_y);
    cpy_r_r0 = CPyTagged_StealAsObject(cpy_r_y);
    cpy_r_r1 = PySet_Discard(cpy_r_x, cpy_r_r0);
    CPy_DecRef(cpy_r_r0);
    if (unlikely(cpy_r_r1 < 0)) {
        CPy_AddTraceback("foo.py", "fossss", 4, CPyStatic_globals);
        goto CPyL2;
    } else
        goto CPyL1;

[TH3CHARLie]

And for the remaining negative_int_emit case(the other one call_negative_magic_emit depends on it), for a simple isinstance statement, it generates following C code:

    int __tmp1 = PyObject_IsInstance(cpy_r_x, cpy_r_r0);
    if (__tmp1 < 0)
        cpy_r_r1 = 2;
    else
        cpy_r_r1 = __tmp1;
    if (unlikely(cpy_r_r1 == 2)) {
        CPy_AddTraceback("foo.py", "check", 8, CPyStatic_globals);
        goto CPyL2;
    } else
        goto CPyL1;

It actually checks if the temp is < 0 and then check the error value(ERR_MAGIC), which I believe may be replaced with a single < 0 check in this PR? @JukkaL

[JukkaL]

The overall approach looks good. This also simplifies the generated code, which is nice.

One detail that I noticed is that it would be better to use the int type instead of Py_ssize_t for the return value of PySet_Discard. In the low-level IR converting an integer to a different width should be made explicit. It's fine to do this in a separate PR, since we don't have a way to represent int yet.

It actually checks if the temp is < 0 and then check the error value(ERR_MAGIC), which I believe may be replaced with a single < 0 check in this PR?

Yeah, the PyObject_IsInstance case can be modified to only do a single check, and after that we can narrow down the result to a bool. (Getting rid of the conversion to bool in simple cases would be nice, but it's out of scope for now.)

[TH3CHARLie]

Well, some strange errors running run.test

running build_ext
building 'native' extension
creating build/temp.macosx-10.15-x86_64-3.7
creating build/temp.macosx-10.15-x86_64-3.7/build
clang -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk -I/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/usr/include -I/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/System/Library/Frameworks/Tk.framework/Versions/8.5/Headers -I/Users/th3charlie/Documents/mypy/mypyc/lib-rt -I/usr/local/include -I/usr/local/opt/[email protected]/include -I/usr/local/opt/sqlite/include -I/Users/th3charlie/Documents/venv/include -I/usr/local/Cellar/python/3.7.7/Frameworks/Python.framework/Versions/3.7/include/python3.7m -c build/__native.c -o build/temp.macosx-10.15-x86_64-3.7/build/__native.o -O3 -Werror -Wno-unused-function -Wno-unused-label -Wno-unreachable-code -Wno-unused-variable -Wno-unused-command-line-argument -Wno-unknown-warning-option
creating build/lib.macosx-10.15-x86_64-3.7
clang -bundle -undefined dynamic_lookup -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk build/temp.macosx-10.15-x86_64-3.7/build/__native.o -L/usr/local/lib -L/usr/local/opt/[email protected]/lib -L/usr/local/opt/sqlite/lib -o build/lib.macosx-10.15-x86_64-3.7/native.cpython-37m-darwin.so
copying build/lib.macosx-10.15-x86_64-3.7/native.cpython-37m-darwin.so -> 

*** Exit status: -11

compile these testcases using mypyc via command line would work, though.

Updates:
I thought it was caused by the newly introduced int32_t or int64_t, but fallback to int and Py_ssize_t seems no help

[JukkaL]

Left a few comments; not a full review.

[TH3CHARLie]

For the negative_int_emit cases, as we discussed yesterday it's wise to introduce a truncate op, therefore, we can coerce the int return value of the called function to the real boolean value we want for the primitive to have. To achieve that, we'd modify the description and also CallC accordingly. But what confuses me is where should we place the coerce/truncate. I'd think of two options if we follow the style we do with call_negative_bool_emit, however, none of them seems to work without breaking the design.

1. add this coercion as part of CallC so that CallC with coercion has two processes. Call a function and then truncate. The IR would be like

r0 :: int32_t
r1 :: bool
r0 = PyObject_IsInstance(obj1, obj2)
r1 = r0

But the problem is clear that direct coercion provides no help. To keep the current semantics, we need an additional branch to finally know whether it should be r0 or error_value to set into r1.

2. add this coercion in exception handling. We can introduce a more complicated error check logic for negative_int_emit, like firstly check and truncate the int return value to bool and then check it for error. But this would make the primitive has no reasonable value until all this completes(recall that we want a bool value from this primitive, but until all this comparison->truncate->error-check logic completes we can only have an int value returned from the c function).

@JukkaL

[JukkaL]

add this coercion as part of CallC so that CallC with coercion has two processes. Call a function and then truncate. The IR would be like

I think that call_c can generate the CallC op followed by a truncate op (the truncate op would be a new RegisterOp subclass that never generates an error). We can rely on the exception handling transform to add a negative check after the call. The final IR would look something like this (without refcount handling):

r0 :: int32_t
r1 :: bool
r0 = PyObject_IsInstance(obj1, obj2)
if r0 < 0 goto ...
r1 = trunc r0 int32_t to bool

[TH3CHARLie]

r0 :: int32_t
r1 :: bool
r0 = PyObject_IsInstance(obj1, obj2)
if r0 < 0 goto ...
r1 = trunc r0 int32_t to bool

If we have python code like and the change you mentioned:

result = isinstance(x, str)

How should we make sure(which part to change) that result gets r1 instead of r0 if we generate two ops: CallC and Truncate, we actually need to reference the Truncate's value in python code like this.

Updates:
maybe a link from CallC to its following Truncate?

[TH3CHARLie]

Ohh that's bad, we have both 32/64 bit CI platforms but the IR tests text are fixed(int64 for now).

[JukkaL]

Thanks for the updates, looks good now! Just a few minor comments.

[JukkaL]

Hmm some tests are failing because the types are different on 32-bit platforms. To work around this, you can make it possible to represent an integer type that varies by the register width in test cases (e.g. native_int); this can be replaced (in the test case description) by the test runner with int32 or int64, depending on the platform. Alternatively, we can skip some of the tests on 32-bit platforms, but this is not as good, since testing these types is actually useful.

The test runner is in test_irbuild.py (and there are others like that, such as test_exceptions.py, that may also need to be changed).

[JukkaL]

Great, looks good!

[JukkaL]

Since the same code is repeated twice, it would make sense to move this into a helper function. This can be done in a separate PR and isn't really necessary, since it's only two lines of code.