Big update to prettier-extension-monkeyc

flocsy said:

I found a bug in 2.0.16

Should be fixed in v2.0.17.

Ok it works now. However there's one more place to improvement:

My original code was:

(:inline)
function setConfig(key as PropertyKeyType, val as PropertyValueType) as Void {
    Properties.setValue(key, val);
}

             setConfig("i" + emptyIndex, antId);
             setConfig("n" + emptyIndex, name);
             setConfig("n", "");

And the optimized code is:

            {
                var key = "i" + emptyIndex;
                Properties.setValue(key, antId);
            }
            {
                var key = "n" + emptyIndex;
                Properties.setValue(key, name);
            }
            {
                Properties.setValue("n", "");
            }

Which IS better in many ways, but if the key parameter is only used once then adding a local variable for it is actually an unnecessary overhead, so IMHO this could be not very hard to optimize 1 step further if the function param is only used once and it is RHS, and then we would get the same code that you would get by manually inlining it:

            {
                Properties.setValue("i" + emptyIndex, antId);
            }
            {
                Properties.setValue("n" + emptyIndex, name);
            }
            {
                Properties.setValue("n", "");
            }

I see now warnings (INFO) during the optimization:
INFO: fenix6: source/Config.mc:55,17: While inlining getConfig: This function can only be inlined in statement, assignment, or return contexts

I thought I understood this, but clearly I haven't :)

So this is the code:

(:api2, :memory32K, :inline)
function getConfig(key as PropertyKeyType) as Value {
    return Properties.getValue(key);
}

and it's being used 3 times in the same way:

function getConfigBool(key as PropertyKeyType, defaultValue as Boolean) as Boolean {
    var value = getConfig(key);
    if (value != null && !(value instanceof Lang.Boolean)) {
        if (value instanceof Lang.Number) {
            value = value != 0;
        } else if (value instanceof Lang.String) {
            value = value.length() > 0;
        }
    }
    if (value == null) {
        value = defaultValue;
    }
    return value;
}

The other 2 places are basically the same, they do slightly different things after the call of getConfig(key), but all 3 are the same in that getConfig(key) is "used in assignment" and BTW also key is only used in this 1 place in all 3 functions. Why this isn't inlined? Which part of the condition in the warning message is not met?

flocsy said:

However there's one more place to improvement

Yes. I did put some effort into dealing with this. As you show, it does exactly what you want for 'setConfig("n", "");'.

But for arbitrary expressions, there is more to it than just how many times the parameter is used.

Consider this (rather contrived) example:

var x = 0;
function f() { x++; return x; }
(:inline)
function g(n) {
  x += n;
  return x;
}
function foo() {
  g(f());
}

Now if we just naively substitute f() for n, we get:

x += f();

rather than;

var n = f(); x += n;

Whats the difference? 

The first one evaluates x (0), evaluates f() (1) then adds 1 to 0, and stores it in x. So x ends up 1.

The second (correct) one, evaluates f() (1), evaluates x (1) then adds 1 to 1 and stores it in x. So x ends up 2.

Or what about

(:inline)
function foo(x) {
  x++;
}
function bar() {
  foo(1);
}

x is only used once, but you can't just substitute the 1 for x.

or

(:inline)
function f(x, y) {
  y[0]++;
  return x;
}
function foo() {
  var x = [1,2];
  f(x[0], x);
}

But yes, I am already checking for this kind of thing - because you *have* to check for it, just to get the cases I already handle right. But right now I only optimize if the parameter is a literal, an identifier or a member expression (a.b). I decided it wasn't worth going after more complex expressions because a) it would only be worthwhile if they were only used once, and b) I'd like to write more general optimizations that take care of this (ie rather than not creating the variable in the first place, I'd rather have an optimization that can eliminate the variable when its safe to do so, regardless of whether it was in the original source, or created by inlining).

flocsy said:

I thought I understood this, but clearly I haven't

Well, thats not assignment context (but yes, it took me a minute to realize that). Thats a variable declaration with an initializer. It's significantly different at the ast level, and would need its own special handling the same way that assignments and return statements did.

However, I don't understand why it *needs* to be in assignment context. That function is a perfect example of a function that simply returns an expression. So I don't understand why it fails to inline, or why it reports that it needs to be in assignment context. I'll see if I can repro...

Meanwhile separating it into a declaration and an assignment should fix it.

var value;

value = getConfig(key);

(unless there's some other reason its not inlining). Note that this is essentially what I will have to do to make the inliner work with declarations.

> " it would only be worthwhile if they were only used once"

well, not exactly, this is used 3 times:

(:api2, :memory32K, :inline) // no inline:c:8868,d:3850 inline:c:8861,d:3829
function getConfig(key as PropertyKeyType) as Value {
    return Properties.getValue(key);
}

BTW I got the following:
ERROR: Internal: Error: Comment "foo" was not printed. Please report this error!
on this code:

(:no_api2, :memory16K, :no_inline) // foo
function setConfig(key as PropertyKeyType, val as PropertyValueType) as Void {
    getApp().setProperty(key, val);
}

flocsy said:

it would only be worthwhile if they were only used once

The "it" in that sentence referred to the argument to the function.

ie it's only better to use a complex expression directly (even when its safe to do so), rather than to assign it to a variable, if the parameter it's replacing is only used once. Adding a variable only seems to add one byte to the code size.

flocsy said:

by splittiong it to: "var value; value = getFonfig(key)"

I found the bug that prevented it from being inlined as an expression. With that fixed, the function works as originally written. I may hold off pushing a fix and roll in some more changes.

flocsy said:

ERROR: Internal: Error: Comment "foo" was not printed

Thanks - I guess it deleted the function, but not the comment in that case. Was setConfig the last function in the file by any chance? Or at least, the last one that didn't get removed...

One more strange thing:

this works:

(:no_api2, :memory16K, :inline)
function getConfig(key as PropertyKeyType) as Value { //n: 4869,2975,3363 i: 4860,2954
    // return getApp().getProperty(key);

    // since this is the only place we used getApp() inlining saves 25 bytes in code and 18 bytes in data:
    // return Application.getApp().getProperty(key);

    // adding using Toybox.Application as App; and using App.Appbase.getProperty saves 3 bytes in code:
    return App.AppBase.getProperty(key);
}

and gets inlined when used as: var value; value = getConfig(key);

However it's setter counterpart fails to compile after optimization:

(:no_api2, :memory16K, :inline)
function setConfig(key as PropertyKeyType, val as PropertyValueType) as Void {
    // getApp().setProperty(key, val);
    // Application.getApp().setProperty(key, val);
    App.AppBase.setProperty(key, val);
}

ERROR: fenix3: X/bin/optimized/group001-debug/source/source/MyApp.mc:139: Undefined symbol "App" detected.

This is because the above code is in Config.mc, where I have: using Toybox.Application as App; but in MyApp.mc I don't have that using statement. When I add that to MyApp.mc then it works.

However I found another bug nested inline doesn't remove the outer inlined function:

(:no_api2, :memory16K, :inline)
function setConfig(key as PropertyKeyType, val as PropertyValueType) as Void {
    App.AppBase.setProperty(key, val);
}

    public function onSettingsChanged() as Void {
        // some code...

        saveAlias();

        // some code...
    }

    (:inline)
    hidden function saveAlias() as Void {
        // long code
                if (emptyIndex != null) {
                    setConfig("i" + emptyIndex, antId);
                    setConfig("n" + emptyIndex, name);
                    setConfig("n", "");
                }
    }

becomes:

    public function onSettingsChanged() as Void {
        // some code

        {
            // long code
                    if (emptyIndex != null) {
                        {
                            var key = "i" + emptyIndex;

                            App.AppBase.setProperty(
                                key,

                                antId
                            );
                        }

                        {
                            var key = "n" + emptyIndex;

                            App.AppBase.setProperty(
                                key,

                                name
                            );
                        }

                        {
                            App.AppBase.setProperty(
                                "n",

                                ""
                            );
                        }
                    }
        }
        // some code
    }

    (:inline)
    hidden function saveAlias() as Void {
        // long code
                if (emptyIndex != null) {
                    {
                        var key = "i" + emptyIndex;
                        App.AppBase.setProperty(key, antId);
                    }
                    {
                        var key = "n" + emptyIndex;
                        App.AppBase.setProperty(key, name);
                    }
                    {
                        App.AppBase.setProperty("n", "");
                    } // set sa to empty string so next time we don't need to do all this in saveAlias()
                    // System.println("saveAlias: " + emptyIndex + ". " + antId + " = " + name);
                }
    }

> Was setConfig the last function in the file by any chance?

No, actually it's the 1st function in the file :)

flocsy said:

No, actually it's the 1st function in the file

Yes, and I misread. Its *not* an inline function, so I assume its not being eliminated...