[Wasm GC] [GUFA] Add initial ConeType support #5116

kripken · 2022-10-05T17:30:41Z

A cone type is a PossibleContents that has a base type and a depth, and it
contains all subtypes up to that depth. So depth 0 is an exact type from
before, etc.

This only adds cone type computations when combining types, that is, when we
combine two exact types we might get a cone, etc. This does not yet use the
cone info in all places (like struct gets and sets), and it does not yet define roots
of cone types, all of which is left for later. IOW this is the MVP of cone types that
is just enough to add them + pass tests + test the new functionality.

This improves the j2wasm binary, but only very slightly.

kripken · 2022-10-05T19:34:15Z

Also, I think this would be easier to read if the two PossibleContents being combined or intersected were symmetric.

That can be a little less readable, I agree. The idea though behind this convention is that it's very common to do .combine(other) and .intersect(other) etc. where we modify an existing contents, and it avoids depending on the optimizing compiler to avoid a copy.

It might be worth measuring this more in detail and possibly refactoring it, though.

tlively · 2022-10-05T19:47:12Z

Looking into both those changes later sounds fine to me.

kripken · 2022-10-05T21:42:51Z

The idea though behind this convention is that it's very common to do .combine(other) and .intersect(other) etc. where we modify an existing contents, and it avoids depending on the optimizing compiler to avoid a copy.

Oh, btw, as more background, this is common in game engines where such operations on vectors and matrices etc. must be very optimized. It's possible it matters less here...

tlively · 2022-10-05T21:46:58Z

The idea though behind this convention is that it's very common to do .combine(other) and .intersect(other) etc. where we modify an existing contents, and it avoids depending on the optimizing compiler to avoid a copy.

Oh, btw, as more background, this is common in game engines where such operations on vectors and matrices etc. must be very optimized. It's possible it matters less here...

I would be very interested to see how much of a difference it makes.

aheejin

Sorry for the delayed reply. Haven't read everything yet, but wonder what would be an example of the case when we know a precise depth for a cone type and what benefit it can bring.

aheejin · 2022-10-06T09:46:30Z

src/ir/possible-contents.h

@@ -133,6 +145,11 @@ class PossibleContents {
  // contents here will then include whatever content was possible in |other|.
  void combine(const PossibleContents& other);

+  // Removes anything not in |other| from this object, so that it ends up with
+  // only their intersection. At this only handles an intersection with a full


Not sure what does "At this" mean here... Is that a typo?

Sorry, that should be "Atm" for "at the moment"... will fix.

kripken · 2022-10-06T16:59:41Z

I should have added an example. Something like this:

struct A { field : i32 };
struct B : A;
struct C : B;

a = new A{.field = 42};
b = new B{.field = 42};
c = new C{.field = 1337};

foo((x ? a : b).f);       // => foo(42)
                          // since Cone(A, 1)
                          // includes only A, B

The input to foo is a cone of limited depth, and that limited depth lets us optimize. If the cone were any larger, we'd include another type which has a different value that prevents optimization.

tlively · 2022-10-06T22:51:08Z

FYI I think this will have a merge conflict with #5115 since the handling of nulls in possible-contents needed to change. I don't have a preference for what order they land in, though.

kripken · 2022-10-06T22:52:49Z

I don't have a preference either. As your PR is much larger, maybe best to let yours land first.

aheejin · 2022-10-07T06:48:51Z

src/ir/possible-contents.h

+
+  // Returns whether the relevant cone for this, as computed by getCone(), is of
+  // full size, that is, includes all subtypes.
+  bool hasFullCone() const { return getCone().depth == FullDepth; }


FullCone sounds good to me.

aheejin · 2022-10-07T07:12:27Z

src/ir/possible-contents.cpp

+    if (!isNull()) {
+      value = mixInNull(getCone());
      return;
-    } else if (!other.isNull() && other.hasExactType()) {
-      value = ExactType(Type(otherType.getHeapType(), Nullable));
+    } else if (!other.isNull()) {
+      value = mixInNull(other.getCone());
      return;


What if the heap type of the two types don't have a lub?

Good catch, yeah, looks like this "raced" with the null type changes that just landed. Fixed.

aheejin · 2022-10-07T08:12:34Z

src/ir/possible-contents.cpp

+      // TODO: we could make a single loop that also does the LUB, at the same
+      // time, and also avoids calling getDepth() which loops once more?


Why do we need a loop?

getDepth() does a loop, basically it goes up the chain of supertypes until the end. So each getDepth() here is a loop, sadly.

aheejin · 2022-10-07T08:20:36Z

src/ir/possible-contents.cpp

+      Index depthUnderLub = depthFromRoot - lubDepthFromRoot + depth;
+      Index otherDepthUnderLub =
+        otherDepthFromRoot - lubDepthFromRoot + otherDepth;


Does a heap type have a unique depth?

It's been a while since I last looked at wasm-type.cpp, so I may lack the context.

binaryen/src/wasm/wasm-type.cpp

Lines 1389 to 1404 in e8884de

size_t HeapType::getDepth() const {

size_t depth = 0;

std::optional<HeapType> super;

for (auto curr = *this; (super = curr.getSuperType()); curr = *super) {

++depth;

}

// In addition to the explicit supertypes we just traversed over, there is

// implicit supertyping wrt basic types. A signature type always has one more

// super, HeapType::func, etc.

if (!isBasic()) {

if (isFunction()) {

depth++;

} else if (isData()) {

// specific struct types <: data <: eq <: any

depth += 3;

}

Here we follow supertypes using a loop and then add +1 if it is a function and +3 if it is a data. But is it possible for one of the supertypes traversed by the for loop is function or data? In that case, is the depth unique?

getSuperType only returns user types atm. So it never returns a basic type like data. (This might be worth improving, but I looked into it and it wasn't trivial.)

The depth should be unique, and is defined to include basic supertypes in the count.

aheejin · 2022-10-07T08:59:24Z

src/ir/possible-contents.cpp

+  auto isSubType = HeapType::isSubType(heapType, otherHeapType);
+  auto otherIsSubType = HeapType::isSubType(otherHeapType, heapType);
+  if (!isSubType && !otherIsSubType) {
+    setNoneOrNull();


Can this be null, given that there's no bottom type (yet)?

Yes, looks like this needs updating as well...

aheejin · 2022-10-07T09:31:04Z

src/ir/possible-contents.cpp

+  // An interesting non-empty intersection that is a new cone which differs from
+  // both the original ones. (This must be an intersection of cones, since by
+  // assumption |other| is a cone, and another cone is the only shape that can
+  // have a non-empty intersection with it that differs from them both.)


Why should this differ from them both? We handled the case of isSubContents(other, *this) above, but can't this be isSubContents(*this, other), so the intersection is just *this? (The code seems to handle this case though)

Correct, thanks. This comment was stale.

src/ir/possible-contents.cpp

aheejin · 2022-10-07T10:05:22Z

src/ir/possible-contents.cpp

+
+  auto type = getType();
+  auto otherType = other.getType();
+  auto heapType = type.getHeapType();


The case of None and Many are handled above, but can this be a Literal or Global? In that case, what it doesn't have a heap type?

This does look unclear, I'll add a comment. The reason is the intersection with another reference type is not empty, so this must be a reference type itself.

aheejin · 2022-10-07T10:14:51Z

src/passes/GUFA.cpp

+      // returns 1 if the input is of a subtype of the intended type, that is,
+      // we are looking for a type in that cone of types.
+      auto intendedContents =
+        PossibleContents::fullConeType(Type(curr->intendedType, NonNullable));


Would be helpful to have a short comment on why this is NonNullable (i.e., otherwise it can trap)

aheejin · 2022-10-07T10:25:50Z

test/gtest/possible-contents.cpp

  assertCombination(exactFuncref, exactAnyref, many);
  assertCombination(exactFuncref, anyGlobal, many);
-  assertCombination(exactFuncref, nonNullFunc, many);
+  assertCombination(exactFuncref, nonNullFunc, coneFuncref1);


Why is nonNullFunc a subtype depth 1 of funcref? It looks we treat all signatures in the same way. Is that gonna be true after implementing the real subtyping for functions?

binaryen/src/wasm/wasm-type.cpp

Lines 1731 to 1736 in e8884de

bool SubTyper::isSubType(const Signature& a, const Signature& b) {

// TODO: Implement proper signature subtyping, covariant in results and

// contravariant in params, once V8 implements it.

// return isSubType(b.params, a.params) && isSubType(a.results, b.results);

return a == b;

}

I think that's right, this is the current temporary situation. @tlively would know for sure.

Co-authored-by: Heejin Ahn <[email protected]>

aheejin · 2022-10-07T21:11:04Z

test/gtest/possible-contents.cpp

+#if BINARYEN_TEST_DEBUG
+  if (!PossibleContents::haveIntersection(a, b) ||
+      !PossibleContents::haveIntersection(b, a)) {
+    std::cout << "\nFailure: no intersection:\n" << a << '\n' << b << '\n';
+    abort();
+  }
+#endif


Why do we need this? If this is the case, doesn't this crash in EXPECT_TRUE above anyway?

EXPECT_TRUE doesn't print out the values being operated on, so we just get "expected true on line 286" and that's it. We don't even know which call to this function caused the problem. The extra logging makes it easy to debug these.

kripken added 30 commits September 26, 2022 16:14

start

51cb0b9

format

6dc7292

builds

ec0b3d6

src/

617690a

fix

f750d67

typo

a34e470

clarify comment

afe8b5d

works

3af1107

rename

c7c6b93

moar + fix one

fa7a116

fix

7927fbe

fix

e504b0b

progress

9a057dc

comment

932da84

Merge remote-tracking branch 'origin/main' into cone

e164411

start

f50a8d4

format

973586d

test

8261dc0

hash index too

650ce4e

considered best

c0aa7e4

Merge remote-tracking branch 'origin/gufa.has' into cone

3a1dbfd

test

85f31a5

Merge remote-tracking branch 'origin/main' into cone

81bcf3e

test

43da163

test

dee3da0

test

c6bc347

test

47fffc4

test

1773856

test

af84e99

test

36ebd4c

kripken added 2 commits October 5, 2022 12:41

rename

9737075

cleanup

1f272ab

tlively approved these changes Oct 5, 2022

View reviewed changes

aheejin reviewed Oct 6, 2022

View reviewed changes

fix typo

b280cc1

aheejin reviewed Oct 7, 2022

View reviewed changes

kripken and others added 12 commits October 7, 2022 11:24

Merge remote-tracking branch 'origin/main' into cone

6f182f8

null lub handling after recent null changes

85e2ac3

test update

415a8ea

fix

05ea12a

fix

d87fc2b

clenup

78002dd

clenup

fd520f8

fix comment

2e0e687

Update src/ir/possible-contents.cpp

6926cb1

Co-authored-by: Heejin Ahn <[email protected]>

Merge remote-tracking branch 'origin/cone' into cone

9cfdc3f

comment

9117223

comment

83d720a

aheejin approved these changes Oct 7, 2022

View reviewed changes

Merge remote-tracking branch 'origin/main' into cone

a1245ad

kripken enabled auto-merge (squash) October 11, 2022 20:09

kripken merged commit 5129f88 into main Oct 11, 2022

kripken deleted the cone branch October 11, 2022 20:41

		// TODO: we could make a single loop that also does the LUB, at the same
		// time, and also avoids calling getDepth() which loops once more?

	size_t HeapType::getDepth() const {
	size_t depth = 0;
	std::optional<HeapType> super;
	for (auto curr = this; (super = curr.getSuperType()); curr = super) {
	++depth;
	}
	// In addition to the explicit supertypes we just traversed over, there is
	// implicit supertyping wrt basic types. A signature type always has one more
	// super, HeapType::func, etc.
	if (!isBasic()) {
	if (isFunction()) {
	depth++;
	} else if (isData()) {
	// specific struct types <: data <: eq <: any
	depth += 3;
	}

	bool SubTyper::isSubType(const Signature& a, const Signature& b) {
	// TODO: Implement proper signature subtyping, covariant in results and
	// contravariant in params, once V8 implements it.
	// return isSubType(b.params, a.params) && isSubType(a.results, b.results);
	return a == b;
	}

[Wasm GC] [GUFA] Add initial ConeType support #5116

[Wasm GC] [GUFA] Add initial ConeType support #5116

Uh oh!

Conversation

kripken commented Oct 5, 2022

Uh oh!

kripken commented Oct 5, 2022

Uh oh!

tlively commented Oct 5, 2022

Uh oh!

kripken commented Oct 5, 2022

Uh oh!

tlively commented Oct 5, 2022

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!

kripken commented Oct 6, 2022

Uh oh!

tlively commented Oct 6, 2022

Uh oh!

kripken commented Oct 6, 2022

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!

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!

Uh oh!