Wrong evaluation order for binary operators

[trikko]

import std.stdio;

void main()
{

    size_t inc(size_t* v)
    {
        (*v)++;
        return 10;
    }


    size_t pos = 10;

    immutable tst = pos + inc(&pos); // <-- this
    writeln(tst);
    writeln(pos);
}

This code outputs:
21
11

Instead of:
20
11

With lcd 1.0.0 alpha

[kinke]

Compile-time only, right?

[trikko]

I don't think so.

[dnadlinger]

It's not compile-time only. Also, from this it seems like we codegen the arguments in LTR order, but fail to immediately load from the pos lvalue:

* * * * * * * DtoVarDeclaration(vdtype = immutable(int))
* * * * * * * * llvm value for decl:   %tst = alloca i32, align 4
* * * * * * * * expression initializer
* * * * * * * * AssignExp::toElem: tst = pos + inc(& pos) | (immutable(int))(immutable(int) = immutable(int))
* * * * * * * * * VarExp::toElem: tst @ immutable(int)
* * * * * * * * * * DtoSymbolAddress ('tst' of type 'immutable(int)')
* * * * * * * * * * * a normal variable
* * * * * * * * * AddExp::toElem: pos + inc(& pos) @ immutable(int)
* * * * * * * * * * VarExp::toElem: pos @ int
* * * * * * * * * * * DtoSymbolAddress ('pos' of type 'int')
* * * * * * * * * * * * a normal variable
* * * * * * * * * * CallExp::toElem: inc(& pos) @ int
* * * * * * * * * * * VarExp::toElem: inc @ int(int* v)
* * * * * * * * * * * * DtoSymbolAddress ('inc' of type 'int(int* v)')
* * * * * * * * * * * * * FuncDeclaration
* * * * * * * * * * * DtoCallFunction()
* * * * * * * * * * * * Building type: int(int* v)
* * * * * * * * * * * * * DtoFunctionType(int(int* v))
* * * * * * * * * * * * * * x86-64 ABI: Transforming return type
* * * * * * * * * * * * * * x86-64 ABI: Transforming argument types
* * * * * * * * * * * * * * Final function type: i32 (i32*)
* * * * * * * * * * * * DtoNestedContext for test.main.inc
* * * * * * * * * * * * * DtoCreateNestedContextType for test.main.inc
* * * * * * * * * * * * * for function of depth -1
* * * * * * * * * * * * * function does not have context or creates its own from scratch, returning undef
* * * * * * * * * * * * doing normal arguments
* * * * * * * * * * * * Arguments so far: (1)
* * * * * * * * * * * * * i8* undef
* * * * * * * * * * * * Function type: int(int* v)
* * * * * * * * * * * * DtoArgument
* * * * * * * * * * * * * SymOffExp::toElem: & pos @ int*
* * * * * * * * * * * * * * DtoSymbolAddress ('pos' of type 'int')
* * * * * * * * * * * * * * * a normal variable
* * * * * * * * * performing normal assignment (rhs has lvalue elems = 0)
* * * * * * * * * DtoAssign()
* * * * * * * * * * lhs:   %tst = alloca i32, align 4
* * * * * * * * * * rhs:   %3 = add i32 %2, %1

That's as far as I got while waiting on the airport; probably won't have time to follow up on it anytime soon.

[kinke]

I think the problem is that we're doing DValue *lv = toElem(l), then DValue *rv = toElem(r) (in ToElemVisitor::visit(AddExp*)), and only then lv->getRVal() + rv->getRVal() (the latter in DtoBinAdd()).
The load of pos happens in lv->getRVal(), while inc() is already called in toElem(r) (I guess).
So we'd probably need to let DtoBinXXX() take 2 LLValue* instead of DValue*, for toElem(l)->getRVal() and then toElem(r)->getRVal().
There may be more issues like this lurking in the depths...

[kinke]

Well maybe using a specialization of DValue for function calls, DCallValue or something, which only lazily evaluates the call expression on the first getLVal()/getRVal() call, would be a more elegant and robust approach to this...

[JohanEngelen]

@kinke I think your proposed fix is correct.

I mean this one:

So we'd probably need to let DtoBinXXX() take 2 LLValue* instead of DValue*, for toElem(l)->getRVal() and then toElem(r)->getRVal().

[kinke]

So we've just had 2 DValue related evaluation-order issues (1 already fixed by Johan). Iirc, @klickverbot mentioned a few months ago that he's generally not happy with DValue. So we may need to re-think this for a proper solution. I don't have a sufficient overview yet and sadly generally not a lot of time right now (just bought an apartment, got the keys today, will be moving in next week etc.).

[dnadlinger]

To fix this and related issues, I think what we need to do is to go through the code base and make sure that toElem is (almost) never called without also immediately invoking get{L, R}Val().

Anybody? ;P

[kinke]

I was thinking about introducing a DtoLVal()/DtoRVal() and hiding toElem. Which obviously also means no more getRVal() and getLVal() for DValue. It simply knows if it's a lvalue or not, i.e., whether its LLValue is a pointer or an immediate.
This would also get rid of quite a few unnecessary loads in the IR, as we sometimes access getRVal() multiple times (e.g., 1st time to get the LLVM type etc.). But I don't have enough time right now.

Edit: DtoRVal() would be somewhat special, in the sense of not always returning an rvalue. If it's a struct or static array, we always keep it in memory, so it would return an lvalue DValue*. This matches the current semantics in DVarValue::getRVal(). Detecting this case (e.g., to perform a memcpy() instead of a store) is trivially accomplished by asking DValue::isLVal().