opOp evaluation order difference with DMD (e.g. `+=`)

[JohanEngelen]

Consider the following code:

int sum;

int return1_add9tosum()
{
    sum += 9;
    return 1;
}

void main()
{
    sum = 0;
    sum += return1_add9tosum();

    import std.stdio;
    writeln(sum);
}

DMD 2.071 writes 10. LDC writes 1. It's an evaluation order difference. The spec says that sum should only be evaluated once; left-to-right evaluation is specced, but then the spec also says that DMD does not comply with left-to-right evaluation of AssignExpressions.
Commit a3adb40 of #1562 changed LDC's behavior (before it behaved like DMD).

With fibers in play, the evaluation order is important even if foo() does not modify sum in sum += foo(). When foo() yields, another fiber might modify sum, and thus an older version of sum will be used when the fiber is resumed. I'm pretty sure this is what happened in Weka's unittest failure. In light of that, I think DMD's evaluation order makes more sense (it would make "+=" do the intuitive thing when using fibers).

[JohanEngelen]

@kinke What do you think?

Edit: I made a forum post about it, I think DMD's spec should be clarified on this point. https://forum.dlang.org/post/tpucyhjhiwkcxajuuzqg@forum.dlang.org

[JohanEngelen]

Deadalnix:

DMD is correct on that one. The semantic is such as :

int plusEqual(ref int a, int b) {
  a = a + b;
  return a;
}

[kinke]

DMD should have a test for such frigging cases! One may try to put that stuff into words for an official spec, but a unittest says more than 100 words. Their test coverage is simply miserable. I explicitly added a test for LDC's current behavior (ldc-developers/dmd-testsuite@030de81 - v *= inc(v) + v;).

So this means that v *= ... isn't equivalent to v = v * ... as in this case, v isn't to be loaded immediately.

[JohanEngelen]

DMD should have a test for such frigging cases!

Yes :((

[JohanEngelen]

@kinke Could you work on a fix?

[kinke]

My motivation is pretty low at the moment, so I wouldn't mind if someone else took care of this crap.

[JohanEngelen]

OK, no prob. I'll see what I can do!
Thanks for the pointer to the explicit test that apparently is wrong.

[kinke]

@JohanEngelen The crucial part is currently here. So when visiting OpAssignExpressions, we pre-evaluate the lvalue subexpression of the lhs (here trivially v, but would be v for (v += 10) *= ... as well), then create a new corresponding OpExpression and codegen that (v * (inc(c) + v)), and finally assign the result to the lvalue.
This doesn't work anymore as regular OpExpressions need to evaluate lhs before rhs. But for OpAssignExpressions, it needs to be reversed (DtoRVal(rhsExpression), then DtoRVal(lhsDLValue)). So we won't be able to codegen a fitting OpExpression - we'll have to call the functions in gen/binops.cpp directly (incl. complex numbers).

[JohanEngelen]

@kinke Yep, I am currently working on it and I came to the same conclusion.
Actually, I very much dislike that we create a piece of AST on-the-fly and codegen it, because it interferes for example with PGO or coverage. So it's good to refactor the code, although tedious.
I also found evaluation order issues with bitwise operators (e.g. sum = sum ^ foo();), those should be fixed. Now the big work of making "+=" work.

[JohanEngelen]

(@kinke thanks for the confirmation btw. I was not entirely sure it was the right course of action)

[kinke]

You're welcome, thanks for taking the time to fix this and apparently other eval order issues. ;)

[JohanEngelen]

And another eval order problem: (val += 7) += mul11ret3(val);.
Fixed too!