Multivalue type creation and inspection by tlively · Pull Request #2459 · WebAssembly/binaryen

tlively · 2019-11-21T00:07:30Z

Adds the ability to create multivalue types from vectors of Type::ValueType.
Only concrete value types are allowed in multivalue type tuples. All types are
transparently interned, so their representation is still a single
uint32_t. Types can be extracted into vectors of ValueTypes, and all the value
types expand into vectors containing themselves.

Multivalue types are not yet used in the IR, but the creation and inspection
functionality is exposed in the C and JS APIs and used in tests.

aheejin

Mostly LGTM, some minor comments

aheejin · 2019-11-21T10:53:54Z

+  size_t operator()(const std::vector<wasm::Type::ValueType>& types) const {
+    size_t res = 0;
+    for (auto vt : types) {
+      res ^= hash<int>{}(vt);


unsigned int?

aheejin · 2019-11-21T10:54:08Z

+  size_t getNumValueTypes() const;
+  const std::vector<ValueType> getValueTypes() const;
+
+  bool isValueType() { return id >= i32 && id < _last_value_type; }


So multivalue types return false for this? Multivalues are also values. Maybe isSimpleValueType..? (Not sure about the name)

I mean "value type" in the sense used by the spec, which is always a single type.

aheejin · 2019-11-21T11:20:14Z

+  return typeLists[id].size();
+}
+
+const std::vector<Type::ValueType> Type::getValueTypes() const {


Is there a missing & in the return type?

No, returning a reference to the vector would not be thread-safe because it could be moved by a different thread allocating a new type. I suppose it's worth storing pointers to vectors instead of directly storing vectors to avoid this problem.

I see. But then does this have to be const?

aheejin · 2019-11-21T11:21:35Z

+      std::vector<Type::ValueType> vts = type.getValueTypes();
+      std::string res("(");
+      for (size_t i = 0; i < vts.size() - 1; ++i) {
+        res += ", ";


Doesn't this print something like (, type1, type2, ...)?

aheejin · 2019-11-21T11:28:25Z

+    std::cout << "  {\n";
+    std::cout << "    BinaryenType " << array << "[] = {";
+    for (size_t i = 0; i < numTypes; ++i) {
+      std::cout << int(valueTypes[i]);


aheejin · 2019-11-21T11:28:31Z

+    }
+    std::cout << "};\n";
+    std::cout << "    BinaryenTypeCreate(" << array << ", " << numTypes
+              << "); // " << int(result) << "\n";


aheejin · 2019-11-21T11:29:37Z

+    std::cout << "  }\n";
+  }
+
+  return uint32_t(int(result));


Why double cast? Isn't Types cast operand to uint32_t in the first place?

Thanks for catching these. I originally had the implicit conversion go to int and didn't update these when I changed it.

kripken · 2019-11-21T17:22:11Z

    exnref,
+    _last_value_type,
    // none means no type, e.g. a block can have no return type. but unreachable
    // is different, as it can be "ignored" when doing type checking across


this comment can be removed completely

kripken · 2019-11-21T17:23:17Z

+  size_t getNumValueTypes() const;
+  const std::vector<ValueType> getValueTypes() const;
+
+  bool isValueType() { return id >= i32 && id < _last_value_type; }


Isn't this check the same as isConcreteType?

No, isConcreteType includes multivalue types as well.

I see, thanks.

How about isSingleType? "value" doesn't seem clear enough to me.

(Also, may as well be isSingle, as the whole thing is a type?)

kripken · 2019-11-21T17:24:02Z

 constexpr Type unreachable = Type::unreachable;

-const char* printType(Type type);
+const std::string printType(Type type);


does this change have an impact on performance? we may call this many many times in a huge wast.

I believe that in the common case performance should still be good, but I'm not entirely sure. I expect to clean up this printing to use ostreams in a follow up PR, so I don't think it's worth blocking on here.

kripken · 2019-11-21T17:26:36Z


+  explicit Type(const std::vector<ValueType>& types);
+
+  size_t getNumValueTypes() const;


How about getNumValues or getArity which are shorter?

Nice, I like getArity.

Adds the ability to create multivalue types from vectors of Type::ValueType. Only concrete value types are allowed in multivalue type tuples. All types are transparently interned, so their representation is still a single uint32_t. Types can be extracted into vectors of ValueTypes, and all the value types expand into vectors containing themselves. Multivalue types are not yet used in the IR, but the creation and inspection functionality is exposed in the C and JS APIs and used in tests.

- Make Type method names more succinct - Use Type instead of ValueType in creation and inspection APIs - Make Type printing much more ergonomic - Use unique_ptr to safely return const references to type vectors - Make common Type predicates methods - Address miscellaneous comments

tlively · 2019-11-22T05:10:54Z

I have no idea why the behavior of wasm-reduce changed :/

kripken · 2019-11-22T17:24:13Z

Running that test locally to see how the output of wasm-reduce changes with this PR might help. I can help do that if you want.

kripken

Basically lgtm aside from those questions. Nice work!

kripken · 2019-11-22T19:01:02Z

+};

-  bool operator!=(const ValueType& other) { return id != other; }
+struct ParamType {


Are ParamType and ResultType just for printing purposes? If so maybe add a comment. Or is there another use?

Yes, I'll add comments.

kripken · 2019-11-22T19:03:36Z

+  size_t operator()(const std::vector<wasm::Type>& types) const {
+    size_t res = 0;
+    for (auto vt : types) {
+      res ^= hash<uint32_t>{}(vt);


where is this hash from? (we have src/support/hash.h which ensures the same hash values across libcs, but this doesn't look like it?)

This is std::hash. I will add the namespace to disambiguate for the reader.

tlively requested a review from kripken November 21, 2019 00:07

aheejin reviewed Nov 21, 2019

View reviewed changes

kripken reviewed Nov 21, 2019

View reviewed changes

tlively added 3 commits November 21, 2019 20:08

Add <cassert>

f40092e

tlively force-pushed the multivalue-types branch from 715b302 to f40092e Compare November 22, 2019 05:10

Fix

6d1da4a

kripken reviewed Nov 22, 2019

View reviewed changes

kripken approved these changes Nov 22, 2019

View reviewed changes

Comments and namespaces

f8cfc80

tlively merged commit e2587f3 into WebAssembly:master Nov 22, 2019

tlively mentioned this pull request Jan 7, 2020

Switch warnings no longer work #2572

Closed

tlively deleted the multivalue-types branch April 24, 2020 23:19


		explicit Type(const std::vector<ValueType>& types);

		size_t getNumValueTypes() const;

Conversation

tlively commented Nov 21, 2019

Uh oh!

aheejin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively commented Nov 22, 2019

Uh oh!

kripken commented Nov 22, 2019

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aheejin Nov 21, 2019 •

edited

Loading