These are chat archives for dropbox/pyston

25th
Aug 2015
Sun
@Daetalus
Aug 25 2015 01:53

Hi, I just fixed the wrong order about complex cfg.

Seems +(1+6j) have two steps:

    #0 = 1+6.000000j
    #1 = +(#0)

But -(1+6j) just has one step(CPython has two step):

    #0 = -1+6.000000j

I check the code in cfg.cpp, seems it use same remapUnaryOp, I have no idea why USub could not get work here.

Kevin Modzelewski
@kmod
Aug 25 2015 02:07
is it right when coming out of the parser?
Sun
@Daetalus
Aug 25 2015 06:25
The output of the parser was correct
Kevin Modzelewski
@kmod
Aug 25 2015 06:53
I guess I meant, maybe it's not cfg.cpp's fault?
it might be interesting to see the ast pre-cfg.cpp
Marius Wachtler
@undingen
Aug 25 2015 08:22
@kmod and @rudi-c thanks for fixing the issue :+1:
Rudi Chen
@rudi-c
Aug 25 2015 19:09
class RewriterVar;
struct Value {
    union {
        bool b;
        int64_t n;
        double d;
        Box* o;
    };
    RewriterVar* var;

    operator RewriterVar*() { return var; }

    Value() : o(0), var(0) {}
    Value(bool b, RewriterVar* var) : b(b), var(var) {}
    Value(int64_t n, RewriterVar* var) : n(n), var(var) {}
    Value(double d, RewriterVar* var) : d(d), var(var) {}
    Value(Box* o, RewriterVar* var) : o(o), var(var) {}
};
What is that special kind of custom operator called?
It's like some way of specifying coercion.
Marius Wachtler
@undingen
Aug 25 2015 19:14
you mean the operator RewriterVar*()?
Rudi Chen
@rudi-c
Aug 25 2015 19:14
Yup
Marius Wachtler
@undingen
Aug 25 2015 19:15
this just implicit returns the var member whenever it sees a cast to RewriterVar*
Rudi Chen
@rudi-c
Aug 25 2015 19:15
Ohhh casts
TIL C++ has custom cast operators
Marius Wachtler
@undingen
Aug 25 2015 19:16
This can be quite dangerous but I added it because it's nice to not have always to write v.var when a function expects a RewriterVar*
Before the bjit got upstreamed all functions used to pass Value than I changed them to accept RewriterVar* and this would have required to add everywhere .var but I didn't like it and the increase of charachters per line it required thats why I added it
Rudi Chen
@rudi-c
Aug 25 2015 19:19
kk makes sense
Sun
@Daetalus
Aug 25 2015 19:25

@Daetalus I think something like this should fix the parsing issue:

ResultPtr read(pypa::AstComplex& c) {
        AST_Num* imag = new AST_Num();
        location(imag, c);
        imag->num_type = AST_Num::COMPLEX;
        sscanf(c.imag.c_str(), "%lf", &imag->n_float);
        if (!c.real)
            return imag;
        AST_BinOp* binop = new AST_BinOp();
        location(binop, c);
        binop->op_type = AST_TYPE::Add;
        binop->right = readItem(c.real, interned_strings);
        binop->left = imag;
        return binop;
    }

does this give you the correct result? (you will have to change the python source file / touch it)

Hi @undingen , last time you write this function solved the complex value in ast. I switched the binop->right and binop->left. Then the order become correct in cfg.

But -(1+6j) always return -1+6j. Seems the USub only affect the "real part" of binop.(I try to assign the imag to binop->left, but USub still could not effect it).

@kmod suggest me to see the ast pre-cfg.cpp, now I know how to see the ast output of pypa(Which was correct), and the cfg output. But don't know how to see it between then.

Would you mind to give some helps about what cause the problem. I drive into cfg.cpp and pypa-parser.cpp, no progress yet. Thanks!

Marius Wachtler
@undingen
Aug 25 2015 19:29
yes, I will look into it in a few minutes
Sun
@Daetalus
Aug 25 2015 19:30
Thanks, just if you have time.
Sun
@Daetalus
Aug 25 2015 19:39
^just -> only :smile:
Marius Wachtler
@undingen
Aug 25 2015 19:49
look like pypa introduces the problem
for print -(1+6j) I'm getting
           [Print]
              - destination: <NULL>
              - newline: True
              - values: [                    
                    [Complex]
                      - imag: +6j
                      - real:                         
                        [Number]
                          - integer: -1 
                    ]
so the negation is already folded into the real number...
Sun
@Daetalus
Aug 25 2015 19:51
Yes, but the outout of libpypa's astdump.py is correct...
Marius Wachtler
@undingen
Aug 25 2015 19:54
I think the astdump.py script uses cpython for parsing and is used to generate lipypa compatible output in order to generate tests for libpypa
Sun
@Daetalus
Aug 25 2015 19:54
Ah... understood.
Sorry for the mistake...
Marius Wachtler
@undingen
Aug 25 2015 19:56
no problem, there are so many different ast/parsing etc steps that it's not obvious who produces what
I think this is wrong
first the whole complex optimization should probably be inside the the perform_inline_optimizations check
Sun
@Daetalus
Aug 25 2015 20:06
And inner else should change to if (p->imag) , and extract the ast = p out?
Marius Wachtler
@undingen
Aug 25 2015 20:06

and then the else {

                    else {
                        p->imag = '-' + p->imag;
                        ast = p;
                    }

shoud get removed and the condition always executed

Sun
@Daetalus
Aug 25 2015 20:06
I see.
Marius Wachtler
@undingen
Aug 25 2015 20:08
not that I have tried it but I expect this to fix the problem :-D. But I'm not sure if we even want this inline optimizations so may be just put it inside the if and disable them for pyston
or just remove the buggy complex optimization.... so many options :-)
Sun
@Daetalus
Aug 25 2015 20:14
I think create an issue or PR maybe is acceptable, not change it directly in Pyston. Is that correct?
Marius Wachtler
@undingen
Aug 25 2015 20:16
yes exactly, open the PR at https://github.com/vinzenz/libpypa/ and when merged create a pyston PR which updates the libpypa submodule to the new HEAD
thanks for working on all that stuff :-)
Sun
@Daetalus
Aug 25 2015 20:19

Thanks for your team's patiently helps!

@vinzenz seems is busy in recently.