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

Implement virtual overloading / interfaces #1208

Merged
merged 29 commits into from
Apr 27, 2020
Merged

Implement virtual overloading / interfaces #1208

merged 29 commits into from
Apr 27, 2020

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Apr 3, 2020

An experiment on a vtable implementation, for discussion, that does not require any additional memory nor indirect calls. Idea is to analyze the program upfront, recording potential virtual calls during compilation, and injecting the vtables as a finalization step. Just a proof-of-concept at this point, not bothering with edge cases, properties or interfaces yet.

class Foo {
  a<T>(a: T): void {}
}

class Bar extends Foo {
  a<T>(a: T): void {}
}

var foo: Foo = new Bar();
foo.a<i32>(1);

late-compiles Bar#a<i32> and injects the following into Foo#a<i32>:

if (rtId == idof<Bar>()) {
  Bar#a<i32>(thisArg, arg0);
} else {
  assert(rdId == idof<Foo>());
  // Foo#a<i32> code
}

Also see the various TODO comments. Other notes:

  • Think about trampolines, which after splitting yield four functions for a single function in the worst case (actual function, vtable'd function, trampoline, vtable'd trampoline). Might want to inline the trampoline into the actual function, even if this means that providing all arguments does a nop switch to keep the code simple.
  • Think about overloading fields with properties and vice-versa. These have different semantics, with only property accessors retaining for the caller. We'll either want to compile virtual fields as properties implicitly or disallow overloading between these. Might even go as far as to disallow overloading of fields, and requiring properties, as we can't inject a vtable into field loads anyway.
  • This is nominal currently, and I haven't though about the implications of making it structural yet.

cc @willemneal

Todos:

  • Implement property overloading
  • Check that abstract and interface members are implemented

fixes #693
fixes #1209
fixes #862
fixes #459

@MaxGraey
Copy link
Member

MaxGraey commented Apr 3, 2020

Relate to #693

src/program.ts Outdated Show resolved Hide resolved
@MaxGraey
Copy link
Member

MaxGraey commented Apr 4, 2020

if (rtId == idof<Bar>()) {
  Bar#a<i32>(thisArg, arg0);
} else {
  assert(rdId == idof<Foo>());
  // Foo#a<i32> code
}

But this make sense only if we have one one level of subclassing (Bar ⇒ Foo). With more than one level + speculative opt it should be like (Bar ⇒ Foo1 ⇒ ... ⇒ FooN) I guess:

if (rtId == idof<Bar>()) {
  Bar#a<i32>(thisArg, arg0);
} else {
  foo.a<i32>(thisArg, arg0); // virtual / indirect call or classid lookup
}

@dcodeIO
Copy link
Member Author

dcodeIO commented Apr 4, 2020

Not sure I understand the objection. Final code I have in mind for an inheritance graph of

Foo
 Bar
  Baz  // not virtual
 Other // not virtual

for Foo#a is

switch (rtId) {
  case Bar_ID: return Bar#a<i32>(thisArg, arg0);
  case Baz_ID: return Baz#a<i32>(thisArg, arg0);
  case Other_ID: return Other#a<i32>(thisArg, arg0);
}
// Foo#a<i32> code

and for Bar#a is

switch (rtId) {
  case Baz_ID: return Baz#a<i32>(thisArg, arg0);
}
// Bar#a<i32> code

where we can skip the vtable in either of them if not called explicitly, and with the switch being generated as a sequence of br_ifs, with Binaryen transforming these to a br_table only if it's ok under the given optimization options.

We can do the same for abstract and interface methods, just that // XY code is an unreachable there.

@dcodeIO
Copy link
Member Author

dcodeIO commented Apr 4, 2020

Last commit builds such a table now. Last time I checked Binaryen attempted to make a br_table from it with more than three br_ifs, but it will only do so if it doesn't blow up code size due to holes between the ids, according to opt levels. Very similar to how normal switches are compiled.

(func $class-overloading/Foo#a<i32>|virtual (; 13 ;) (param $0 i32) (param $1 i32)
  (local $2 i32)
  block $self
   block $id3
    block $id4
     local.get $0 ;;
     i32.const 8  ;;
     i32.sub      ;; load this's id and store in a temp
     i32.load     ;;
     local.set $2 ;;
     local.get $2
     i32.const 4
     i32.eq
     br_if $id4   ;; break to class id 4 overload if id == 4
     local.get $2
     i32.const 3
     i32.eq
     br_if $id3   ;; break to class id 3 overload if id == 3
     local.get $2
     i32.const 5
     i32.eq
     br_if $self  ;; break to self if id == 5
     unreachable  ;; otherwise trap (invalid id)
    end
    local.get $0
    local.get $1
    call $class-overloading/Bar#a<i32>
    return
   end
   local.get $0
   local.get $1
   call $class-overloading/Baz#a<i32>
   return
  end
  local.get $0
  local.get $1
  call $class-overloading/Foo#a<i32>
 )

@willemneal
Copy link
Contributor

Yeah the above looks very similar to what I produced, except that it was a branch table. Looking over the code, it wouldn't be that much to add interfaces and abstract methods.

@dcodeIO
Copy link
Member Author

dcodeIO commented Apr 5, 2020

Last commit now uses the Relooper to build the table, but intentionally avoids emitting a br_table right away in case ids are distant (addBranch instead of addBranchForSwitch etc.). This appears to have the advantage that if there are let's say 10 class ids mapping to just two overloads (+ original), Binaryen can optimize that to two branches using bitwise i32.ors, which is quite size-efficient especially if ids are distant, but also seems to prevent the entire thing from becoming a br_table after optimization if we are only interested in speed. However, considering that there's already an optimization for sequences of br_ifs to a br_table, I might either be missing something or, if not, it'd be a logical addition to Binaryen to also support that for code generated using the Relooper's addBranch. Code there typically looks like multiple levels of

(local.set $0 (...))
(if
 (i32.or
  (i32.or
   (i32.eq
    (local.get $0)
    (i32.const 1)
   )
   (i32.eq
    (local.get $0)
    (i32.const 2)
   )
  )
  (i32.eq
   (local.get $0)
   (i32.const 3)
  )
 )
 (call ...) ;; or another if
 (call ...) ;; or unreachable
)

where at some point a br_table will be more optimal if ids are dense.

Also accounts for several edge cases now, like virtual methods being inherited by an extendee before being (not) overridden again, where I figured that if we make the original function the fallback without asserting the remaining valid ids, we can skip a bunch of code checking all those ids.

@dcodeIO
Copy link
Member Author

dcodeIO commented Apr 6, 2020

Fiddling around with bootstrapping revealed that we can parse stdlib, rt-stub and a simple entry file, even initialize the program, but compilation ultimately fails with a varargs bug, which seems to be a problem with calling trampolines with arguments that again call trampolines, so argumentsLength becomes confused.

@dcodeIO
Copy link
Member Author

dcodeIO commented Apr 8, 2020

There are some interface bits now as well, with parts of it largely based on Willem's earlier work in #862. Still missing proper checks, and a pass on properties, though.

@dcodeIO
Copy link
Member Author

dcodeIO commented Apr 17, 2020

Looking tasty. If anyone is specifically interested in landing this asap, you can help me by trying to break it with additional tests that should pass, but don't.

protected c: i32; // ERROR 1042: "'protected' modifier cannot be used here."
public c: i32; // ERROR 1042: "'public' modifier cannot be used here."
static d: i32; // ERROR 1042: "'static' modifier cannot be used here."
constructor(): i32; // ERROR 229: "'constructor' keyword cannot be used here." (TS: none?)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to allow new for constructors like ts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Not much to gain functionality, since one can't pass around classes to call new on them dynamically. But perhaps solely for compat / type checking?

@dcodeIO dcodeIO changed the title Proof-of-concept vtable Implement virtual overloading / interfaces Apr 27, 2020
@dcodeIO
Copy link
Member Author

dcodeIO commented Apr 27, 2020

Merging, so others can get their hands on it as well. There'll very likely be additional PRs based on feedback.

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

Successfully merging this pull request may close these issues.

A mechanism for virtual overloads Minimum viable virtual methods
3 participants