Issue 1827 - Inline does not faithfully copy variables at identical times - haxe

[issuesbot]

[Google Issue #1827 : https://code.google.com/p/haxe/issues/detail?id=1827]
by gameh...@gmail.com, at 18/05/2013, 08:52:25

The following gived different results depending where 'set' is inlined. The actucal bug came from matrix multiplication.
http://code.google.com/p/hxcpp/issues/detail?id=241

class Test

{
    var a:Int;
    var b:Int;
    function new() { a=1; b=2; }
    inline function set(inA:Int, inB:Int)
    {
        a = inA;
        b = inB;
    }
    public static function main()
    {
        var t = new Test();
        t.set(t.b,t.a);
        trace(t.a +"," +t.b);
    }
}

Maybe this should just be 'implementer beware'.

[issuesbot]

[comment from si...@haxe.org, published at 18/05/2013, 09:06:54]
Ouch, that looks a bit bad.

var t = new Main();

{
    t.a = t.b;
    t.b = t.a;
}

;

[issuesbot]

[comment from si...@haxe.org, published at 23/05/2013, 10:25:02]

[issuesbot]

[comment from ncanna...@gmail.com, published at 23/05/2013, 10:36:03]
I wonder how much we should enforce this.

ATM, inline differs from traditional functions in that the time at which arguments are evaluated is delayed until it's actually used in the method. Unused arguments are even discarded, which is great if you want for instance to have assert(expr) that entirely disappear in release.

I would vote for a good explanations in the Haxe 3.0 Manual about this particular way inline can break code.

[issuesbot]

[comment from si...@haxe.org, published at 23/05/2013, 10:41:40]
I disagree that documentation is the solution for this particular problem. It silently introduces logic errors which typically don't even imply runtime errors. It's not hard to imagine lengthy debugging sessions to find out why certain values are wrong.

[issuesbot]

[comment from thomas.p...@gmail.com, published at 23/05/2013, 10:46:17]
@ Simon : indeed, that was my case before I've been able to isolate the problem and fill an issue.

I really think that inline should be working in a secure way.

If you really want to offer a higher level of inlining, which can be cool, I'd suggest to not make it by default, but in a specific compiler argument like "--inline-depth 2" or whatever.

Thomas

[issuesbot]

[comment from david.el...@gmail.com, published at 23/05/2013, 10:51:58]
I agree with simon. Game dev relies heavily on inlining, it shan't be unsafe.

[issuesbot]

[comment from si...@haxe.org, published at 23/05/2013, 10:54:48]
Thomas, why do you mention inline depth? The problem here is caused by a singular inline.

[issuesbot]

[comment from ncanna...@gmail.com, published at 23/05/2013, 11:16:15]
I'll open again and think about it. That would need to force storing all ".field" parameters in a temp var instead of inlining its value at the use-place, which is a bit annoying since it might create accesses that would be been removed otherwise.

For instance :

inline function doIf( flag : Bool, a : Int, b : Int ) {
    return if( flag ) a else b;
}

doIf(true,obj.foo,obj.bar);

currently compiles to "obj.foo"

would compile to :

{
    var _tmp1 = obj.foo;
    var _tmp2 = obj.bar;
    _tmp1;
}

[issuesbot]

[comment from ncanna...@gmail.com, published at 23/05/2013, 11:16:53]

[issuesbot]

[comment from si...@haxe.org, published at 23/05/2013, 11:19:36]
I agree that finding a way to inline it properly is a bit annoying, but I think we could at least detect it and cancel inlining.

[issuesbot]

[comment from si...@haxe.org, published at 23/05/2013, 11:48:06]
My idea would be to

1. mark all this.fields which is assigning an expression containing a local and then
2. cancel inlining if such a field is replaced back.

[issuesbot]

[comment from ncanna...@gmail.com, published at 24/05/2013, 08:06:49]
a) you don't want to cancel inlining, you just need to put the value in a temp var
b) That might also break for this.field.a = obj.b; this.field.b = obj.a; when obj == this.field

[issuesbot]

[comment from ncanna...@gmail.com, published at 24/05/2013, 09:32:20]
I don't think we should rush out things. Inlining has not changed much since the beginning, so I guess solving this can wait a few more weeks.

[kiroukou]

@Simn I mentioned that idea because current inline implementation is a bit too strong, since if not used with real care, it can lead to some very frustrating debugging session. I really think most of people experimenting such a problem do not fill issues, thinking it was their mistake (which currently is)
@ncannasse The output with temp variable example looks like the expected default behaviour to me :) (event if less optimized).

That's why I'd go for a default inlining algorithm safer, but that we can change using a compiler flag to the current algorithm.

[sledorze]

Do we agree on 'correct' then 'fast'?
Optimization should be opportunistic and consistent rather than systematic
and buggy.
(easy to say)

On Fri, May 31, 2013 at 10:32 AM, kiroukou notifications@github.com wrote:

@Simn https://github.com/Simn I mentioned that idea because current
inline implementation is a bit too strong, since if not used with real
care, it can lead to some very frustrating debugging session. I really
think most of people experimenting such a problem do not fill issues,
thinking it was their mistake (which currently is)
@ncannasse https://github.com/ncannasse The output with temp variable
example looks like the expected default behaviour to me :) (event if less
optimized).

That's why I'd go for a default inlining algorithm safer, but that we can
change using a compiler flag to the current algorithm.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/1827#issuecomment-18732417
.

Stéphane Le Dorze

http://lambdabrella.blogspot.com/
http://www.linkedin.com/in/stephaneledorze
http://twitter.com/stephaneledorzehttps://twitter.com/#!/stephaneledorze/status/74020060125077504

Tel: +33 (0) 6 08 76 70 15

[Atry]

Does this bug cause https://code.google.com/p/hxformat/issues/detail?id=55 ?

[ncannasse]

In order to keep inline power and still deal with the issue we will have to perform some global optimization as suggested in #2135 , we will push that to the same release then

[Simn]

Here's another one:

class Main {
    static function main() {
        var i = 0;
        var k = call(i, i++);
        trace(k); // 0

        var i = 0;
        var k = callInline(i, i++);
        trace(k); // 1
    }

    static function call(i1, i2) {
        return i1;
    }

    static inline function callInline(i1, i2) {
        return i1;
    }
}

The inliner doesn't create a temp var for the first argument because the argument itself doesn't have any side-effects. The generated code then increments before use:

var k1;
{
    var i2 = i1 ++;
    k1 = i1;
};
haxe.Log.trace(k1,{fileName : "Main.hx",lineNumber : 9,className : "Main",methodName : "main"});

That's pretty rubbish...

[ncannasse]

If you specified that the order of evaluation of the arguments to inline function is not specified, or that evaluation might actually not occur if optimized out, then it works. We still have the issue with "delayed evaluation" of the original issue example.

[Simn]

But I don't like this specification. :(

[ncannasse]

@Simn that's how it is currently, and I think it allows some nice things. inline is for performance optimisation anyway, so users using it should know the pro/cons and adapt to it, or stick with a standard call.

[andyli]

Still I believe any optimisation should not change the behaviour of a program. The current spec let inlining changes argument evaluation order (from specified to unspecified), which make it more than an optimisation. And I think argument evaluation order is pretty important and should be specified - let's imagine if it is not specified for normal function calls.
If ppl really want that aggressive behaviour, they may use a macro function instead.

[nadako]

I second that. inline should mean that the body of the function is inserted at the call site and nothing more. It's weird to specify behaviour change like that and I believe it's very surprising for 99% of users.
As for optimisations, wouldn't the new analyzer do the right thing for the most of the cases with const propagation and local dce?

[ncannasse]

@nadako : it won't in some cases, such as debug("Value = "+complexCalculation()). Currently if inline actually do nothing, it will also prevent calling the complexCalculation function, which is the pattern we use.
Of course it is now possible to use macros for that, but that's definitely a breaking change wrt to previous usage of inline so I prefer we wait for Haxe 4 to talk again about this one.

[nadako]

While I agree that's a breaking change and we should be careful, I wonder how many people actually RELY on that behaviour besides you :-P I think it takes quite deep knowledge of haxe inlining (which is something only you have as an author) to be 100% sure that complexCalculation call won't be generated (also --no-inline will screw that up).

As a "common" haxe programmer, I'd find that quite unreliable and would just use a simple and straightforward macro for case like yours (debug(complexCalculation())).

Also, I wonder if there are more uses for the "optimization" besides that debug thingie and whether they worth having issues like #3535

[Simn]

I'm confused:

class Main {
    static function main() {
        debug("Value = "+complexCalculation());
    }

    @:extern
    static inline function debug(e) { }

    static function complexCalculation() {
        return "11" + 2;
    }
}

Generates (neko):

class Main{
    static main(method) : Void -> Void

     = function() = {
        var e = "Value = " + Main.complexCalculation();
        null;
    }

    static complexCalculation(method) : Void -> String

     = function() = {
        return "11" + 2;
    }

}

[ncannasse]

@Simn that's a regression, not sure since when

[Simn]

Can we get rid of this feature if it has been broken for more than a year? I'd even go ahead and bisect it. :)

[Atry]

This "feature"?
2015年1月15日 3:29 AM于 "Simon Krajewski" notifications@github.com写道：

Can we get rid of this feature if it has been broken for more than a year?
I'd even go ahead and bisect it. :)

—
Reply to this email directly or view it on GitHub
#1827 (comment)
.

[Simn]

Well surprise: fa24c15

You made that change yourself and even put it in the changelog.

[nadako]

lol

[ncannasse]

Ah, that's indeed quite funny :-) But then I think it still works well for things such as debug("VAR="+var) because neither Binop or Field are considered side effects in that case.

[Simn]

You can't be serious. :P

[ncannasse]

That's what the code tells me, and I even remember documenting it !
"tells if an expression causes side effects. This does not account for potential null accesses (fields/arrays/ops)"

@Simn does the analyser deopt var tmp = "VAR="+this.field; if tmp is not used ?

[ncannasse]

@Simn and what about var tmp = "VAR="+obj.field; (since we can assume that this.field will never cause a null access)

[Simn]

It removes these. Why wouldn't it? Operations on null values are undefined anyway, right?

[MSGhero]

Is this related to this issue? I couldn't find an issue exactly relevant.

Edit: After reinitializing the array, it works fine. The original array had a fixed length of 0 I guess?

[ncannasse]

OK, let's move on with this and have inline respect order of evaluation. I still recommend we directly inline constants (and lambda functions) in-place instead of putting them in temp vars so we keep code optimization the way it is right now without having to enable the analyzer

[Simn]

It's funny because by now I'm leaning towards your original position. The effects of enforcing this are substantial and without a really good optimization pass it is going to cause performance regressions. So I suppose the question is if I'm capable of implementing that really good optimization pass, and given the mutable and dynamic nature of Haxe I'm not so sure about that.

[ncannasse]

Ah-ah !

I agree that until we are sure to be able to perform good optimizations on par with we have we have right now we shouldn't change the behavior. At least we agree now on both what would be nice and how hard it is to get there :)

[Simn]

This is the code example I use to remind myself that it's impossible to statically detect inlining order problems in general:

class Main {
    var a:Int;
    var b:Int;

    function new() {
        a = 1;
        b = 2;
    }

    @:extern inline function set(inA:Int, inB:Int) {
        setA(inA);
        setB(inB);
    }

    function setA(i:Int) a = i;
    function setB(i:Int) b = i;

    static function alias(t:Main) return t;

    public static function main() {
        var t = new Main();
        var t2 = alias(t);
        t.set(t.b, t2.a);
        trace(t.a + "," + t.b);
    }
}

I'm posting this because every other month I come back to this issue and think that I can somehow fix it in the inliner, while that is, in fact, impossible.