Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inline constructors rework #6178

Merged
merged 10 commits into from
May 1, 2017

Conversation

basro
Copy link
Contributor

@basro basro commented Apr 13, 2017

This is a rework of the inline constructors filter which adds support for inlining in multiple new cases.

New constructor inlining features by example:

Nested constructors

var a = { x : ["foo", "bar"], y : 0.5, z : new Point(0,0) };
a.x[0] = "modified";
a.y = 1.5;
trace( a.x[0] + a.x[1] + a.y + a.z.y );

This compiles to (JS target):

var a_z_y;
var a_z_x;
var a_y;
var a_x_1;
var a_x_0 = "foo";
a_x_1 = "bar";
a_y = 0.5;
a_z_x = 0;
a_z_y = 0;
a_x_0 = "modified";
a_y = 1.5;
console.log(a_x_0 + a_x_1 + a_y + a_z_y);

And collapses into just console.log("modified" + "bar" + 1.5 + 0); with analyzer-optimize on.

Multiple aliasing variables:

If a variable is aliasing an inlined object, assigning it to another variable makes that other variable also an alias.

var a = [0, 1, 2];
var b = a;
var c = { x : b, y : "test" };
trace(b[0] + a[1] + c.x[2]);

Unassigned variables:

Variables that aren't assigned in their declaration can alias inlined objects too as long as they are only assigned once and appear only in a restricted scope.

var a;
a = new Point(-5, 10);
trace(a.x + a.y);

This is especially useful for nested inline class constructors, since class fields are declared without initialization and assigned later in the body of the constructor.

0 aliasing variables:

Objects can be inlined even if no variable is aliasing them (as long as they are wrapped by a field access)

trace(new Point(100, 55).x);
trace([1, 2, 4, 8, 16][3]);
trace({x:"hello", y:"world"}.x);

Abstracts don't block inlining anymore:

This is just a side effect of the above mentioned features, but it's the main reason why I started this rework so it's worth mentioning.
Abstract constructors undergo this transformation:

var a = new AbstractPoint(1, 1);

turns into

var this;
this = new Point(1,1);
var a = this;

Previously that code would cancel inlining, but thanks to multiple aliases and unassigned variables being supported, this will now inline properly.
Abstract operator overloading combined with inline constructors make a great combo: fast and ergonomic vector math!

Some tradeoffs

There's a few cases in which my implementation will do worse than the current inline constructors implementation.

Array inline variables are not assigned in declaration

Edit: This is no longer a problem thanks to the improvements Simn did for variable declarations fusing with their first assignment.

In my rework variables are always declared and assigned in separate expressions, but the old inline constructor algorithm manages to declare and assign in a single expression when inlining arrays or anonymous structures.

The reworked version produces equally valid output, but it's a bit bigger.

Note that inlining classes behaves the same in both algorithms.

var a = [0, 1, 2];

Haxe 3.4.2 output:

var a_0 = 0;
var a_1 = 1;
var a_2 = 2;

Reworked output:

var a_2;
var a_1;
var a_0;
a_0 = 0;
a_1 = 1;
a_2 = 2;

Some cases involving Dynamic won't inline

Because of multiple aliasing, I was forced to cancel inlining if field access is typed differently than the field in the inlined expression. This only happens when dynamic is involved.

public static inline function makeStruct() return {x:1};

public static function testInline() {
	var a : {x:Dynamic} = makeStruct();
	a.x = "hi";
	trace(a.x);
}

The inlined object is {x:1}, with x being of type Int. In a.x = "hi" x is used with a different type so inlining is cancelled.
In haxe 3.4.2 this will inline and a_x will have type Dynamic.

Bugfixes

This PR also fixes issues #6135 and #6169.

@Simn
Copy link
Member

Simn commented Apr 18, 2017

To be frank, I have no idea how to review something like this, so here's a suggestion:

  1. Move all of your new code to inline.ml or something.
  2. Keep the current implementation as-is.
  3. Make your new implementation the default, but add a -D old-constructor-inline flag to switch to the old one.

This way all the new stuff is nicely contained within its own file and can be reasoned about independently. If something breaks horribly we can fall back to the old implementation and check diffs.

@basro
Copy link
Contributor Author

basro commented Apr 25, 2017

I've merged the latest haxe development branch in order to make use of the improvements Simn made to the analyzer variable declaration and assignment fusing.
Updated some optimizer tests that had the output change because of it.

I also made it unwrap blocks in some cases, so that less superfluous blocks are produced.

This latest changes however have brought to the light a bug which was present even in the old constructor inliner. The lua tests will fail because of it.

I've found that the reason the bug happens is that functions with untyped expressions are marked with @:has_untyped metadata, but when inlining a constructor untyped expressions are inserted into functions that didn't have that metadata.

I'm not 100% sure of how to fix this, is it possible to modify the metadata of the function in which the filter is running?

@Simn
Copy link
Member

Simn commented Apr 25, 2017

I've found that the reason the bug happens is that functions with untyped expressions are marked with @:has_untyped metadata, but when inlining a constructor untyped expressions are inserted into functions that didn't have that metadata.

I'm not 100% sure of how to fix this, is it possible to modify the metadata of the function in which the filter is running?

I sort of knew about this but decided to ignore this case for the time being. We could set ctx.curfield in run_expression_filters similar to how it already sets ctx.curclass. The filter can then access it accordingly.

But man, I really wish we didn't have to do this... untyped really is a curse.

@basro
Copy link
Contributor Author

basro commented Apr 26, 2017

I added curfield to run_expression_filters and implemented passing the has_untyped metadata.

@Simn Simn merged commit 105387f into HaxeFoundation:development May 1, 2017
@Simn
Copy link
Member

Simn commented May 1, 2017

Thanks a lot for this!

@basro
Copy link
Contributor Author

basro commented May 1, 2017

Hurray!

@Gama11
Copy link
Member

Gama11 commented May 1, 2017

Those two issues mentioned can probably be closed now? (#6135 and #6169)

@Simn
Copy link
Member

Simn commented May 1, 2017

Do we have tests for them?

@basro
Copy link
Contributor Author

basro commented May 1, 2017

I'll write the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants