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.

Sign up for GitHub

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

Add new compound Signature, Struct and Array types #3012

Merged
merged 57 commits into from
Aug 24, 2020
Merged

Add new compound Signature, Struct and Array types #3012

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
927bd6d
Refactor types to represent arbitrary complex types
dcodeIO Aug 1, 2020
e84664f
fix
dcodeIO Aug 1, 2020
185f887
fix
dcodeIO Aug 1, 2020
415353f
I know nothing
dcodeIO Aug 1, 2020
941bde4
layers
dcodeIO Aug 1, 2020
37eb81f
unify printing
dcodeIO Aug 1, 2020
f4067bd
tuple terminology, Tuple::toString via wrapping
dcodeIO Aug 2, 2020
5146da2
make nullable a conditional property of specific TypeDefs
dcodeIO Aug 2, 2020
7299028
remove now unused function parameters
dcodeIO Aug 2, 2020
a8dce05
move 'null' to TypeDef when printing
dcodeIO Aug 2, 2020
e006f21
ValueType -> ID, efficient lookups
dcodeIO Aug 2, 2020
a41c3d5
Extract kind
tlively Aug 3, 2020
2764af7
rename Type::ID to Type::BasicID
dcodeIO Aug 3, 2020
8e159d1
rename ID -> BasicID in other comments
dcodeIO Aug 3, 2020
f1a25a4
avoid using default cases
dcodeIO Aug 3, 2020
1e901fc
Merge remote-tracking branch 'tlively/dcodeIO-gc-extract-kind' into g…
dcodeIO Aug 3, 2020
34c2b55
fix, remove getKind
dcodeIO Aug 3, 2020
b4ffb98
pass Signature by value
dcodeIO Aug 3, 2020
12183ec
packed field types
dcodeIO Aug 4, 2020
e2f3281
copy assignment for TupleDef
dcodeIO Aug 4, 2020
fbb0897
remove unnecessary Field constructor
dcodeIO Aug 4, 2020
cda1056
add isNullable predicate
dcodeIO Aug 4, 2020
5ba1afc
use Type instead of BasicID in isPacked check
dcodeIO Aug 4, 2020
8bf1906
document TypeDefs of lists of a single type
dcodeIO Aug 4, 2020
eb11e41
fix copy assignment as suggested
dcodeIO Aug 4, 2020
3761ed5
revert separate printing of Field::PackedType
dcodeIO Aug 4, 2020
7da7012
simplify TypeDef copy assignment
dcodeIO Aug 5, 2020
c2cf485
Merge branch 'master' into gc-type-refactor
dcodeIO Aug 15, 2020
06cb1a3
use wasm::hash
dcodeIO Aug 15, 2020
dac491d
Merge branch 'master' into gc-type-refactor
dcodeIO Aug 17, 2020
656f0a3
Merge branch 'master' into gc-type-refactor
dcodeIO Aug 17, 2020
adb4332
drop extra Tuple wrapper, clarify nested Ref concept
dcodeIO Aug 18, 2020
7e46013
Update src/wasm-type.h
dcodeIO Aug 18, 2020
8aa4b5d
Update src/wasm-type.h
dcodeIO Aug 18, 2020
4ed19aa
Update src/wasm-type.h
dcodeIO Aug 18, 2020
fc05734
Update src/wasm/wasm-type.cpp
dcodeIO Aug 18, 2020
94e2f61
Update src/wasm/wasm-type.cpp
dcodeIO Aug 18, 2020
959aba1
Update src/wasm/wasm-type.cpp
dcodeIO Aug 18, 2020
e7eeb50
add getDef for readability, remove PackedType storage type
dcodeIO Aug 18, 2020
b72890c
fix
dcodeIO Aug 18, 2020
a9affcf
keep getDef internal as suggested
dcodeIO Aug 18, 2020
40f3a1a
move TypeDef definition to wasm-type.cpp
dcodeIO Aug 18, 2020
accd33d
initial test
dcodeIO Aug 18, 2020
d92e416
parens it is
dcodeIO Aug 18, 2020
e8e111d
Merge branch 'master' into gc-type-refactor
dcodeIO Aug 20, 2020
9517da1
Update src/wasm-type.h
dcodeIO Aug 20, 2020
c20b61f
add an assert
dcodeIO Aug 20, 2020
f6b9f83
revert Tuple move constructor until expert feedback
dcodeIO Aug 20, 2020
0de9d01
remove unnecessary this reference
dcodeIO Aug 20, 2020
0cdc596
Merge branch 'master' into gc-type-refactor
dcodeIO Aug 20, 2020
85fb78f
move remains of TypeDef to wasm-type.cpp, rename to TypeInfo and impl…
dcodeIO Aug 21, 2020
cecf38a
apply dangerous half-knowledge
dcodeIO Aug 21, 2020
fa620d9
interesting tests
dcodeIO Aug 21, 2020
9d206af
revert dangerous half-knowledge
dcodeIO Aug 23, 2020
b8d6faf
Update src/wasm-type.h
dcodeIO Aug 24, 2020
fd81477
Update src/wasm-type.h
dcodeIO Aug 24, 2020
5d617c9
Merge branch 'master' into gc-type-refactor
dcodeIO Aug 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions src/wasm-type.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ class Type {
explicit Type(const Tuple&);

// Construct from signature description
explicit Type(const Signature, bool nullable);
explicit Type(const Signature&, bool nullable);
dcodeIO marked this conversation as resolved.
Show resolved Hide resolved

// Construct from struct description
explicit Type(const Struct&, bool nullable);
Expand Down Expand Up @@ -143,9 +143,9 @@ class Type {
// otherwise ambiguous whether to convert both this and other to int or
// convert other to Type.
bool operator==(const Type& other) const { return id == other.id; }
bool operator==(const BasicID& other) const { return id == other; }
bool operator==(BasicID otherId) const { return id == otherId; }
dcodeIO marked this conversation as resolved.
Show resolved Hide resolved
bool operator!=(const Type& other) const { return id != other.id; }
bool operator!=(const BasicID& other) const { return id != other; }
bool operator!=(BasicID otherId) const { return id != otherId; }

// Order types by some notion of simplicity
bool operator<(const Type& other) const;
Expand All @@ -165,12 +165,12 @@ class Type {
static Type get(unsigned byteSize, bool float_);

// Returns true if left is a subtype of right. Subtype includes itself.
static bool isSubType(Type left, Type right);
static bool isSubType(const Type& left, const Type& right);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since Types are small anyway we are either pushing its uintptr_t id or an equally sized reference, so that's mostly style, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Side note: binaryen master is still in a weird state where we've changed anyref to externref, but the other reference types changes haven't yet been applied; in this case, there is no longer any subtyping relationship, so I assume this method needs to be refactored out as part of removing nullref, adding an immediate type tag to ref.null, etc.

Apologies if this causes issues for folks, I only had spare cycles to do the initial work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reminder. Plan forward might be:

  1. Complete this PR so we have a foundation for compound types
  2. Remove nullref, add eqref, i31ref and the new anyref alongside initial handling of the new compound types
  3. Think about RTTs
  4. Update ref.null and ref.is_null alongside adding the new GC instructions

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems good to me. Apologies if it was already top of mind 🤡

Copy link
Member

Choose a reason for hiding this comment

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

cc @aheejin, who I think was planning to make some changes to reflect the current state of reference types. It would be good to coordinate with her on who is going to make what changes.

On topic here, I think it would be best to keep passing the types directly here. The extra indirection of passing a reference (which means the values must be in memory rather than in registers) is unnecessary. The only reason we use const Type& for operator== and friends is that those are the standard kinds of types used for those operator overloads.

Copy link
Member

Choose a reason for hiding this comment

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

I have a half-baked (or, more precisely, quarter-baked) patch for bringing reference types proposal up to date in my local machine, which I started some weeks ago and didn't get around to finish. I'm now trying to pick up the patch.. Do you need that changes for GC? I don't want to block you from proceeding to wait for my patch, but at the same time, I think it is good to add that reference type patch separately from GC and make sure all existing (and to-be-updated) tests the fuzzer pass with reference types proposal, because currently the fuzzer is testing reference type instructions already.

I was away for most of the last week. I was gonna make this change this week, but please let me know if your plan is blocked by this or interfere with this schedule, in which case we can change the plan. I wasn't able to take a look at this patch yet, but I think I'll soon.

Copy link
Member

@tlively tlively Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The GC proposal restores anyref and nullref, so all we really need to do right now is add an externref type. But some current uses of anyref might need to use externref instead. @aheejin, you'll have a better idea than me whether it's better to rename the current anyref to externref then add a new anyref as part of the GC work or to just add a new externref to what we already have.

Copy link
Member

@aheejin aheejin Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm confused.

  • Does the GC proposal really restore nullref? After the recent reference type changes, every reference type can be nullified, or in other words, have a null value. Why would we need to restore a separate nullref type? I think what you quoted is just not up-to-date, no?
  • I think we should add anyref; anyref and externref seem to be two separate types. anyref is a supertype of all reftypes while externref is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Iiuc there is no nullref anymore.

Copy link
Member

Choose a reason for hiding this comment

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

Aha, I've been looking at the wrong GC document. Thanks for the clarification, @dcodeIO!


// Computes the least upper bound from the type lattice.
// If one of the type is unreachable, the other type becomes the result. If
// the common supertype does not exist, returns none, a poison value.
static Type getLeastUpperBound(Type a, Type b);
static Type getLeastUpperBound(const Type& a, const Type& b);

// Computes the least upper bound for all types in the given list.
template<typename T> static Type mergeTypes(const T& types) {
Expand Down Expand Up @@ -216,22 +216,23 @@ class Type {
// Wrapper type for formatting types as "(param i32 i64 f32)"
struct ParamType {
Type type;
ParamType(Type type) : type(type) {}
ParamType(const Type& type) : type(type) {}
std::string toString() const;
};

// Wrapper type for formatting types as "(result i32 i64 f32)"
struct ResultType {
Type type;
ResultType(Type type) : type(type) {}
ResultType(const Type& type) : type(type) {}
std::string toString() const;
};

struct Tuple {
TypeList types;
Tuple() : types() {}
Tuple(std::initializer_list<Type> types) : types(types) {}
Tuple(TypeList types) : types(types) {}
Tuple(const TypeList& types) : types(types) {}
Tuple(TypeList&& types) : types(std::move(types)) {}
bool operator==(const Tuple& other) const { return types == other.types; }
bool operator!=(const Tuple& other) const { return !(*this == other); }
std::string toString() const;
Expand All @@ -241,7 +242,8 @@ struct Signature {
Type params;
Type results;
Signature() : params(Type::none), results(Type::none) {}
Signature(Type params, Type results) : params(params), results(results) {}
Signature(const Type& params, const Type& results)
: params(params), results(results) {}
dcodeIO marked this conversation as resolved.
Show resolved Hide resolved
bool operator==(const Signature& other) const {
return params == other.params && results == other.results;
}
Expand All @@ -259,7 +261,7 @@ struct Field {
} packedType; // applicable iff type=i32
bool mutable_;

Field(Type type, bool mutable_ = false)
Field(const Type& type, bool mutable_ = false)
dcodeIO marked this conversation as resolved.
Show resolved Hide resolved
: type(type), packedType(not_packed), mutable_(mutable_) {}
Field(PackedType packedType, bool mutable_ = false)
: type(Type::i32), packedType(packedType), mutable_(mutable_) {}
Expand All @@ -285,7 +287,8 @@ typedef std::vector<Field> FieldList;
struct Struct {
FieldList fields;
dcodeIO marked this conversation as resolved.
Show resolved Hide resolved
Struct(const Struct& other) : fields(other.fields) {}
Struct(FieldList fields) : fields(fields) {}
Struct(const FieldList& fields) : fields(fields) {}
Struct(FieldList&& fields) : fields(std::move(fields)) {}
dcodeIO marked this conversation as resolved.
Show resolved Hide resolved
bool operator==(const Struct& other) const { return fields == other.fields; }
bool operator!=(const Struct& other) const { return !(*this == other); }
std::string toString() const;
Expand All @@ -294,7 +297,8 @@ struct Struct {
struct Array {
Field element;
Array(const Array& other) : element(other.element) {}
Array(Field element) : element(element) {}
Array(const Field& element) : element(element) {}
Array(Field&& element) : element(std::move(element)) {}
bool operator==(const Array& other) const { return element == other.element; }
bool operator!=(const Array& other) const { return !(*this == other); }
std::string toString() const;
Expand Down
21 changes: 12 additions & 9 deletions src/wasm/wasm-type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,13 @@ struct TypeInfo {
ArrayRef arrayRef;
};

TypeInfo(Tuple tuple) : kind(TupleKind), tuple(tuple) {}
TypeInfo(Signature signature, bool nullable)
TypeInfo(const Tuple& tuple) : kind(TupleKind), tuple(tuple) {}
TypeInfo(Tuple&& tuple) : kind(TupleKind), tuple(std::move(tuple)) {}
dcodeIO marked this conversation as resolved.
Show resolved Hide resolved
TypeInfo(const Signature& signature, bool nullable)
: kind(SignatureRefKind), signatureRef{signature, nullable} {}
TypeInfo(Struct struct_, bool nullable)
TypeInfo(const Struct& struct_, bool nullable)
: kind(StructRefKind), structRef{struct_, nullable} {}
TypeInfo(Array array, bool nullable)
TypeInfo(const Array& array, bool nullable)
: kind(ArrayRefKind), arrayRef{array, nullable} {}
TypeInfo(const TypeInfo& other) {
kind = other.kind;
Expand Down Expand Up @@ -281,7 +282,9 @@ static uintptr_t canonicalize(const TypeInfo& info) {
return id;
}

static TypeInfo* getTypeInfo(Type type) { return (TypeInfo*)type.getID(); }
static TypeInfo* getTypeInfo(const Type& type) {
return (TypeInfo*)type.getID();
}

Type::Type(std::initializer_list<Type> types) : Type(Tuple(types)) {}

Expand All @@ -303,7 +306,7 @@ Type::Type(const Tuple& tuple) {
id = canonicalize(TypeInfo(tuple));
}

Type::Type(const Signature signature, bool nullable) {
Type::Type(const Signature& signature, bool nullable) {
id = canonicalize(TypeInfo(signature, nullable));
}

Expand Down Expand Up @@ -415,7 +418,7 @@ Type Type::reinterpret() const {
}

FeatureSet Type::getFeatures() const {
auto getSingleFeatures = [](Type t) -> FeatureSet {
auto getSingleFeatures = [](const Type& t) -> FeatureSet {
TODO_SINGLE_COMPOUND(t);
switch (t.getBasic()) {
case Type::v128:
Expand Down Expand Up @@ -457,7 +460,7 @@ Type Type::get(unsigned byteSize, bool float_) {
WASM_UNREACHABLE("invalid size");
}

bool Type::isSubType(Type left, Type right) {
bool Type::isSubType(const Type& left, const Type& right) {
if (left == right) {
return true;
}
Expand All @@ -479,7 +482,7 @@ bool Type::isSubType(Type left, Type right) {
return false;
}

Type Type::getLeastUpperBound(Type a, Type b) {
Type Type::getLeastUpperBound(const Type& a, const Type& b) {
if (a == b) {
return a;
}
Expand Down