Skip to content

[WasmGC] Heap2Local: Optimize RefCast failures #6727

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

Merged
kripken merged 28 commits into from
Jul 11, 2024
Merged

[WasmGC] Heap2Local: Optimize RefCast failures #6727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
a4e1778
almost
kripken Jun 21, 2024
38f2297
work
kripken Jun 24, 2024
3009f94
work
kripken Jun 24, 2024
10a8b9d
work
kripken Jun 24, 2024
43e5381
Undo.opt
kripken Jun 24, 2024
d36281b
re-do
kripken Jun 24, 2024
1c11a1e
Revert "re-do"
kripken Jun 24, 2024
19823a6
format
kripken Jun 24, 2024
078b813
almost
kripken Jun 24, 2024
090af2d
work
kripken Jun 24, 2024
49d7545
Merge remote-tracking branch 'origin/main' into heap2local.ref.eq
kripken Jun 25, 2024
60050be
fix
kripken Jun 25, 2024
dea6db5
add two more tests
kripken Jun 25, 2024
9029ba2
improve test
kripken Jun 25, 2024
f305e4f
more
kripken Jun 25, 2024
8d26f56
test
kripken Jun 25, 2024
cffde15
todo
kripken Jun 25, 2024
66f8b13
work
kripken Jun 25, 2024
72c9b60
test
kripken Jun 25, 2024
fd71b16
formt
kripken Jun 25, 2024
4193d58
test
kripken Jun 25, 2024
1eb9848
fix
kripken Jun 25, 2024
56d5e98
merge
kripken Jun 26, 2024
584b750
yolo
kripken Jul 3, 2024
3488e5d
fix
kripken Jul 3, 2024
f5a998c
Merge remote-tracking branch 'myself/heap2local.ref.is' into heap2loc…
kripken Jul 3, 2024
d6d4d12
Merge remote-tracking branch 'origin/main' into heap2local.castfailz
kripken Jul 11, 2024
6df1b8b
clarify in a comment
kripken Jul 11, 2024
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
44 changes: 22 additions & 22 deletions src/passes/Heap2Local.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,11 +379,13 @@ struct EscapeAnalyzer {
}

void visitRefCast(RefCast* curr) {
// As it is our allocation that flows through here, we need to
// check that the cast will not trap, so that we can continue
// to (hopefully) optimize this allocation.
if (Type::isSubType(allocation->type, curr->type)) {
escapes = false;
// Whether the cast succeeds or fails, it does not escape.
escapes = false;

// If the cast fails then the allocation is fully consumed and does not
// flow any further (instead, we trap).
if (!Type::isSubType(allocation->type, curr->type)) {
fullyConsumes = true;
}
}

Expand Down Expand Up @@ -783,24 +785,22 @@ struct Struct2Local : PostWalker<Struct2Local> {
return;
}

// It is safe to optimize out this RefCast, since we proved it
// contains our allocation and we have checked that the type of
// the allocation is a subtype of the type of the cast, and so
// cannot trap.
replaceCurrent(curr->ref);
// We know this RefCast receives our allocation, so we can see whether it
// succeeds or fails.
if (Type::isSubType(allocation->type, curr->type)) {
// The cast succeeds, so it is a no-op, and we can skip it, since after we
// remove the allocation it will not even be needed for validation.
replaceCurrent(curr->ref);
Copy link
Member

@tlively tlively Jul 11, 2024

Choose a reason for hiding this comment

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

Even if the cast is known to succeed, we might still need it to make the static types work out, right? This replacement is only valid if curr->ref->type is a subtype of curr->type, not necessarily if allocation->type is a subtype.

This isn't a new issue, so I must be missing something that makes this safe.

Aha, it's because we're going to remove the allocation entirely. This is worth a comment!

Copy link
Member Author

@kripken kripken Jul 11, 2024

Choose a reason for hiding this comment

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

Sounds good, I added a comment.

} else {
// The cast fails, so this must trap.
replaceCurrent(builder.makeSequence(builder.makeDrop(curr->ref),
builder.makeUnreachable()));
}

// We need to refinalize after this, as while we know the cast is not
// logically needed - the value flowing through will not be used - we do
// need validation to succeed even before other optimizations remove the
// code. For example:
//
// (block (result $B)
// (ref.cast $B
// (block (result $A)
//
// Without the cast this does not validate, so we need to refinalize
// (which will fix this, as we replace the unused value with a null, so
// that type will propagate out).
// Either way, we need to refinalize here (we either added an unreachable,
// or we replaced a cast with the value being cast, which may have a less-
// refined type - it will not be used after we remove the allocation, but we
// must still fix that up for validation).
refinalize = true;
}

Expand Down
104 changes: 89 additions & 15 deletions test/lit/passes/heap2local.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2640,24 +2640,34 @@
(type $A (sub (struct (field (ref null $A)))))
;; CHECK: (type $1 (func (result anyref)))

;; CHECK: (type $B (sub $A (struct (field (ref $A)))))
(type $B (sub $A (struct (field (ref $A)))))
;; CHECK: (type $B (sub $A (struct (field (ref $A)) (field i32))))
(type $B (sub $A (struct (field (ref $A)) (field i32))))

;; CHECK: (type $3 (func (result i32)))

;; CHECK: (func $func (type $1) (result anyref)
;; CHECK-NEXT: (local $a (ref $A))
;; CHECK-NEXT: (local $1 (ref $A))
;; CHECK-NEXT: (local $2 (ref $A))
;; CHECK-NEXT: (local $2 i32)
;; CHECK-NEXT: (local $3 (ref $A))
;; CHECK-NEXT: (local $4 i32)
;; CHECK-NEXT: (ref.cast (ref $B)
;; CHECK-NEXT: (block (result (ref $A))
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block (result nullref)
;; CHECK-NEXT: (local.set $2
;; CHECK-NEXT: (local.set $3
;; CHECK-NEXT: (struct.new $A
;; CHECK-NEXT: (ref.null none)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $4
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $1
;; CHECK-NEXT: (local.get $2)
;; CHECK-NEXT: (local.get $3)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $2
;; CHECK-NEXT: (local.get $4)
Copy link
Member Author

@kripken kripken Jul 11, 2024

Choose a reason for hiding this comment

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

These changes are what I meant in the second paragraph - $B has an extra field now, which is also handled here (with value 1). This does not affect what the test is testing.

;; CHECK-NEXT: )
;; CHECK-NEXT: (ref.null none)
;; CHECK-NEXT: )
Expand All @@ -2680,6 +2690,7 @@
(struct.new $A
(ref.null none)
)
(i32.const 1)
)
)
)
Expand All @@ -2688,16 +2699,24 @@

;; CHECK: (func $cast-success (type $1) (result anyref)
;; CHECK-NEXT: (local $0 (ref $A))
;; CHECK-NEXT: (local $1 (ref $A))
;; CHECK-NEXT: (local $1 i32)
;; CHECK-NEXT: (local $2 (ref $A))
;; CHECK-NEXT: (local $3 i32)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block (result nullref)
;; CHECK-NEXT: (local.set $1
;; CHECK-NEXT: (local.set $2
;; CHECK-NEXT: (struct.new $A
;; CHECK-NEXT: (ref.null none)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $3
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $0
;; CHECK-NEXT: (local.get $1)
;; CHECK-NEXT: (local.get $2)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $1
;; CHECK-NEXT: (local.get $3)
;; CHECK-NEXT: )
;; CHECK-NEXT: (ref.null none)
;; CHECK-NEXT: )
Expand All @@ -2711,25 +2730,80 @@
(struct.new $A
(ref.null none)
)
(i32.const 1)
)
)
)
)
;; CHECK: (func $cast-failure (type $1) (result anyref)
;; CHECK-NEXT: (struct.get $B 0
;; CHECK-NEXT: (ref.cast (ref $B)
;; CHECK-NEXT: (struct.new $A
;; CHECK-NEXT: (struct.new $A
;; CHECK-NEXT: (ref.null none)
;; CHECK-NEXT: (local $0 (ref null $A))
;; CHECK-NEXT: (local $1 (ref null $A))
;; CHECK-NEXT: (block ;; (replaces unreachable StructGet we can't emit)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block (result nullref)
;; CHECK-NEXT: (local.set $1
;; CHECK-NEXT: (struct.new $A
;; CHECK-NEXT: (ref.null none)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $0
;; CHECK-NEXT: (local.get $1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (ref.null none)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $cast-failure (result anyref)
(struct.get $B 0
;; The allocated $A arrives here, but the cast will fail, so we do not
;; optimize.
;; The allocated $A arrives here, but the cast will fail. We can remove
;; the allocation and put an unreachable here. (Note that the inner
;; struct.new survives, which would take another cycle to remove.)
(ref.cast (ref $B)
(struct.new $A
(struct.new $A
(ref.null none)
)
)
)
)
)

;; CHECK: (func $cast-failure-nofield (type $3) (result i32)
;; CHECK-NEXT: (local $0 (ref null $A))
;; CHECK-NEXT: (local $1 (ref null $A))
;; CHECK-NEXT: (block ;; (replaces unreachable StructGet we can't emit)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block (result nullref)
;; CHECK-NEXT: (local.set $1
;; CHECK-NEXT: (struct.new $A
;; CHECK-NEXT: (ref.null none)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $0
;; CHECK-NEXT: (local.get $1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (ref.null none)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $cast-failure-nofield (result i32)
;; As above, but we read from a field that only exists in $B, despite the
;; allocation that flows here being an $A. We should not error on that.
(struct.get $B 1 ;; this changes from 0 to 1
(ref.cast (ref $B)
(struct.new $A
(struct.new $A
Expand Down