Remove FunctionType #2510

tlively · 2019-12-06T03:07:12Z

Function signatures were previously redundantly stored on Function
objects as well as on FunctionType objects. These two signature
representations had to always be kept in sync, which was error-prone
and needlessly complex. This PR takes advantage of the new ability of
Type to represent multiple value types by consolidating function
signatures as a pair of Types (params and results) stored on the
Function object.

Since there are no longer module-global named function types,
significant changes had to be made to the printing and emitting of
function types, as well as their parsing and manipulation in various
passes.

The C and JS APIs and their tests also had to be updated to remove
named function types.

Function signatures were previously redundantly stored on Function objects as well as on FunctionType objects. These two signature representations had to always be kept in sync, which was error-prone and needlessly complex. This PR takes advantage of the new ability of Type to represent multiple value types by consolidating function signatures as a pair of Types (params and results) stored on the Function object. Since there are no longer module-global named function types, significant changes had to be made to the printing and emitting of function types, as well as their parsing and manipulation in various passes. The C and JS APIs and their tests also had to be updated to remove named function types.

tlively · 2019-12-06T03:07:49Z

This needs to be rebased, but is otherwise good to go (i.e. tests pass).

kripken · 2019-12-06T03:09:08Z

Great! Starting to read now.

Btw, I'd recommend running the emscripten test suite on this, if you haven't already.

kripken

Excellent work!

Some initial comments until tomorrow.

src/ir/ExpressionAnalyzer.cpp

kripken · 2019-12-06T03:25:33Z

src/ir/ExpressionManipulator.cpp

      for (Index i = 0; i < curr->operands.size(); i++) {
        ret->operands.push_back(copy(curr->operands[i]));
      }
+      ret->type = curr->type;


Is this needed because makeCallIndirect no longer receives the type? I think perhaps it should still receive it. We don't want to risk people forgetting to set it after calling (make* should return a fully-formed node), and in general, where the type can't be inferred from the inputs, we add the type as an extra input (e.g. makeLocalGet, makeCall).

makeCallIndirect performs finalization, which fills out the type using the passed Signature. The problem here is that if any of the operands is unreachable, the type should also be unreachable, but since the operands are added after finalization we need to propagate that information manually.

The code would be cleaner if the operands were passed as part of the builder call so finalization can take care of this. I'll make that change.

src/ir/hashed.h

src/ir/module-utils.h

kripken · 2019-12-06T03:32:44Z

src/ir/module-utils.h

+  typedef std::unordered_map<Signature, size_t> Counts;
+  ModuleUtils::ParallelFunctionAnalysis<Counts> analysis(
+    wasm, [&](Function* func, Counts& counts) {
+      if (func->imported()) {


don't we need imported function's types too?

kripken · 2019-12-06T03:33:16Z

src/ir/module-utils.h

+        return;
+      }
+      struct TypeCounter : PostWalker<TypeCounter> {
+        Module& wasm;


No need for wasm in this entire struct?

kripken

(another set of comments)

kripken · 2019-12-06T17:33:11Z

src/passes/Print.cpp

@@ -1865,18 +1880,18 @@ struct PrintSExpression : public OverriddenVisitor<PrintSExpression> {
    o << ')';
  }
  // Module-level visitors
-  void visitFunctionType(FunctionType* curr, Name* internalName = nullptr) {
+  void visitSignature(Signature curr, Name* funcName = nullptr) {


this looks like a standard visitor, but it isn't, I think? maybe another name would make it less likely to confuse. like handleSignature?

kripken · 2019-12-06T17:42:37Z

src/wasm/wasm-emscripten.cpp

  if (!sigs.insert(sig).second) {
    return; // sig is already in the set
  }
-  Name name = std::string("dynCall_") + sig;
+  Name name = std::string("dynCall_") + "TODO: generate sig string";


What is the status of this TODO?

Embarrassingly skipped. Thankfully it's a trivial fix.

kripken · 2019-12-06T17:47:21Z

src/wasm/wasm.cpp

+bool Function::isVar(Index index) {
+  auto base = getVarIndexBase();
+  // TODO: update me
+  return base <= index /*&& index < base + vars.size()*/;


I think the eventual update here should be to make that last part an assertion - it is invalid to ask avoid an invalid index.

kripken · 2019-12-06T17:49:28Z

test/anyref.wast.from-wast

@@ -1,11 +1,11 @@
 (module
- (type $FUNCSIG$aa (func (param anyref) (result anyref)))
+ (type "anyref -> anyref" (func (param anyref) (result anyref)))


it is slightly unfortunate to print out a type that is not visibly used anywhere, like in this test (i like the change to not print the type on the function, though!)

can be as a followup though if it's not trivial.

I actually don't mind having this section because it's easy to ignore if I want but sometimes it's nice to have an overview of all the function types in one place. But changing this would require a separate ParallelAnalysisPass, so lets defer that discussion to a followup.

… handleSignature

tlively · 2019-12-06T21:47:49Z

Thanks for you comments so far! I have addressed all of them.

kripken

Code looks good to me aside from the issue of the text emitted not being valid wat.

I viewed 25% of the tests and they look fine...

aheejin

Nice simplifying changes!

aheejin · 2019-12-09T10:51:39Z

src/asm2wasm.h

-      if (builtin) {
-        import->type = builtin->name;
+      Signature builtin = getBuiltinSignature(import->module, import->base);
+      if (builtin != Signature()) {


What does an empty signature mean? Does that mean (none, none)? If that's the case, Type::id field currently does not seem to be initialized with 0. Shouldn't we?

On second thought, do we need this condition? Can't we just do

import->sig = getBuiltinSignature(import->module, import->base);

This will include an empty signature case as well.

Yeah, this is rather ugly. Empty signature is indeed (none, none). I thought that was what the default constructor was doing, but I guess I don't know for sure. I'll change it to explicitly initialize params and results to none.

getBuiltinSignature is meant to return an optional signature, and in the absence of std::optional I was just using the default signature to mean the absence of a builtin signature. But this is an ugly hack because there presumably could at some point be a builtin JS function with signature (none, none). I will change this to take a pointer to the result signature and return a bool indicating whether the signature exists. This is a more idiomatic ugly hack.

Hmm, I see. About the default constructor of Type, wouldn't appending = 0; to this line be sufficient?

aheejin · 2019-12-09T11:15:24Z

src/asmjs/asm_v_wasm.cpp

@@ -120,33 +126,6 @@ Type sigToType(char sig) {
  }
 }


Do we still need these getSig and sigToType functions?

getSig still used in wasm-emscripten.cpp, but it turns out that sigToType could be deleted entirely.

aheejin · 2019-12-09T11:30:28Z

src/ir/module-utils.h

+// indices. Signatures are sorted in order of decreasing frequency to minize the
+// size of their collective encoding. Both a vector mapping indices to
+// signatures and a map mapping signatures to indices are produced.
+inline void


Why is this inline?

For the bad reason that it is in a header, so the linker would yell at us if it were not inline because it is included into multiple compilation units. This is a terrible reason, but I feel that adding a new .cpp file for the implementations in this file should be a separate PR.

Oh I see. Yeah that should be a separate PR if we decide to do that, but I don't think that's problem only for this file either (we have many header implementations throughout the codebase), so I'm not suggesting you do that or anything 😂

aheejin · 2019-12-09T11:40:51Z

src/ir/module-utils.h

+    // order by frequency then simplicity
+    if (a.second != b.second) {
+      return a.second > b.second;
+    } else {


Nit: no need for else

aheejin · 2019-12-09T11:42:09Z

src/ir/utils.h

@@ -300,7 +298,7 @@ struct AutoDrop : public WalkerPass<ExpressionStackWalker<AutoDrop>> {
  void doWalkFunction(Function* curr) {
    ReFinalize().walkFunctionInModule(curr, getModule());
    walk(curr->body);
-    if (curr->result == none && curr->body->type.isConcrete()) {
+    if (curr->sig.results == Type::none && curr->body->type.isConcrete()) {


Just curious, when do we prefix types with Type:: and when we don't? Are you gonna land #2434?

I would still like to land #2434, but since I started this before we came to consensus on that, I am just adding Type:: on any line that this PR changes anyway. That will make rebasing and landing #2434 easier afterwards.

So your suggestion is basically wherever we add new code we append Type::?

aheejin · 2019-12-09T13:58:42Z

src/wasm/wasm-s-parser.cpp

  }
+
+  Signature inlineSig{Type(params), Type(results)};


Nit: style issue, but we don't seem to use this {} constructor call in Binaryen. (LLVM prevents it too)

Unforunately this line parses as a function definition if I use parens here 🙄 I guess I can fix it by using an =.

Ah then this should be fine, sorry.

aheejin · 2019-12-09T14:14:19Z

src/wasm/wasm-s-parser.cpp

-  FunctionType* fakeFunctionType; // just co call parseTypeUse
-  i = parseTypeUse(s, i, fakeFunctionType, paramTypes, results);
-  event->sig = Signature(Type(paramTypes), results);
+  i = parseTypeUse(s, i, event->sig);


aheejin · 2019-12-09T14:18:52Z

src/wasm/wasm-validator.cpp

+  if (shouldBeTrue(curr->index < getFunction()->getNumLocals(),
+                   curr,
+                   "local.set index must be small enough")) {
+    ;


aheejin · 2019-12-09T14:20:40Z

src/wasm/wasm-validator.cpp

+      shouldBeEqual(getFunction()->getLocalType(curr->index),
+                    curr->value->type,
+                    curr,
+                    "local.set type must match function");


Why move these into if (shouldBeTrue(curr->index < getFunction()->getNumLocals() ...)? It looks these can be checked independently from the local index failure.

Because it is now an assertion failure to call Function::getLocalType with an out-of-bounds index.

aheejin · 2019-12-09T14:31:24Z

src/wasm2js.h

@@ -411,9 +411,8 @@ Ref Wasm2JSBuilder::processWasm(Module* wasm, Name funcName) {
    asmFunc[3]->push_back(processFunction(


params and vars does not seem to be used anywhere, so we can delete them (not this line but the line above. Github doesn't let me comment on that line...)

nice catch!

…n-types

This allows the signatures to be referenced by index even though they weren't named, which is apparently allowed.

tlively · 2019-12-11T17:11:54Z

I'm currently battling bugs that are only exposed by running the Emscripten test suite (so it's a good thing I'm doing that!). I'll land this once I have those all fixed.

…n-types

This reverts commit 04d88bc.

…n-types

tlively requested review from kripken and aheejin December 6, 2019 03:07

kripken reviewed Dec 6, 2019

View reviewed changes

tlively added 4 commits December 6, 2019 12:58

Simplify code for copying CallIndirects

9edae93

Simplify collectSignatures

75fccaa

Assert on isParam and isVar, remove Function::type. visitSignature ->…

dfd6c3c

… handleSignature

Fix Emscripten dyncall signatures (previously TODO)

570c8d6

kripken approved these changes Dec 6, 2019

View reviewed changes

aheejin reviewed Dec 9, 2019

View reviewed changes

Update signature printing and address comments

8b68944

aheejin mentioned this pull request Dec 10, 2019

Make local.tee's type its local's type #2511

Merged

tlively added 5 commits December 9, 2019 18:26

Address more comments

0e08bc0

Merge branch 'master' of github.com:WebAssembly/binaryen into functio…

85365d5

…n-types

Fix formatting error from CI by deletion

bf0d47c

Fix function name identifiers

aad664c

Add implicit signatures in wast to global list

893c5d1

This allows the signatures to be referenced by index even though they weren't named, which is apparently allowed.

aheejin approved these changes Dec 11, 2019

View reviewed changes

tlively added 3 commits December 11, 2019 13:50

Fix Emscripten bugs

58bf434

Merge branch 'master' of github.com:WebAssembly/binaryen into functio…

80e3637

…n-types

Update tests

04d88bc

tlively mentioned this pull request Dec 11, 2019

Generalize test in prep for binaryen#2510 emscripten-core/emscripten#10011

Merged

tlively added 3 commits December 11, 2019 15:14

Revert "Update tests"

7e5f203

This reverts commit 04d88bc.

Merge branch 'master' of github.com:WebAssembly/binaryen into functio…

7050d8b

…n-types

Update tests

1df40e1

Fix whitespace

f86f020

kripken merged commit 759c485 into WebAssembly:master Dec 12, 2019

tlively mentioned this pull request Dec 17, 2019

Fix isVar to check that the index is valid #2498

Closed

aheejin mentioned this pull request Apr 9, 2020

Performance optimizations for Type #2733

Merged

tlively deleted the function-types branch April 24, 2020 23:16

aheejin mentioned this pull request Aug 24, 2020

Add new compound Signature, Struct and Array types #3012

Merged

		@@ -411,9 +411,8 @@ Ref Wasm2JSBuilder::processWasm(Module* wasm, Name funcName) {
		asmFunc[3]->push_back(processFunction(

Remove FunctionType #2510

Remove FunctionType #2510

Conversation

tlively commented Dec 6, 2019

tlively commented Dec 6, 2019

kripken commented Dec 6, 2019

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlively commented Dec 6, 2019

kripken left a comment

Choose a reason for hiding this comment

aheejin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlively commented Dec 11, 2019