Skip to content

[Custom Descriptors] Fix CFP on an exact ref.get_desc #7886

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+265 −13
Open

[Custom Descriptors] Fix CFP on an exact ref.get_desc #7886

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4ffe0f7
fix
kripken Sep 5, 2025
eca60f9
works on struct.get too, not just ref.get_desc
kripken Sep 5, 2025
590f43e
remove func field
kripken Sep 5, 2025
e42cbb0
use exactness more generally
kripken Sep 6, 2025
c8ad971
Avoid doing more in existing test
kripken Sep 6, 2025
255d0f9
show exact opts right after that one
kripken Sep 6, 2025
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
25 changes: 22 additions & 3 deletions src/passes/ConstantFieldPropagation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,15 @@ struct FunctionOptimizer : public WalkerPass<PostWalker<FunctionOptimizer>> {
// ref.as_non_null (we need to trap as the get would have done so), plus the
// constant value. (Leave it to further optimizations to get rid of the
// ref.)
auto* value = makeExpression(info, heapType, curr);
optimizeSingleValue(info, heapType, curr, ref);
}

void optimizeSingleValue(const PossibleConstantValues& info,
HeapType type,
Expression* curr,
Expression* ref) {
auto* value = makeExpression(info, type, curr);
Builder builder(*getModule());
auto* replacement =
builder.blockify(builder.makeDrop(builder.makeRefAs(RefAsNonNull, ref)));
replacement->list.push_back(value);
Expand Down Expand Up @@ -282,6 +290,11 @@ struct FunctionOptimizer : public WalkerPass<PostWalker<FunctionOptimizer>> {
return;
}

if (refType.isExact() && depth > 0) {
// We do not need to handle subtypes, and this is a subtype.
return;
}

auto iter = rawNewInfos.find(type);
if (iter == rawNewInfos.end()) {
// This type has no struct.news, so we can ignore it: it is abstract.
Expand Down Expand Up @@ -331,8 +344,14 @@ struct FunctionOptimizer : public WalkerPass<PostWalker<FunctionOptimizer>> {
assert(values[0].used() || !values[1].used());

if (!values[1].used()) {
// We did not see two constant values (we might have seen just one, or
// even no constant values at all).
// We did not see two constant values, so this is a simple case that does
// not need a ref.test.
if (values[0].used()) {
// We found exactly one value. This can happen because we consider
// subtyping more carefully than the non-reftest logic (specifically, we
// notice exact types). Optimize to the single possible value.
optimizeSingleValue(values[0].constant, refHeapType, curr, ref);
Copy link
Member

Choose a reason for hiding this comment

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

Can we not make the original analysis more precise by taking exactness into account when propagating information? That seems like it would be more robust than detecting and fixing up the imprecision later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that we aren't fixing it up later: We are reading from rawNewInfos just below, that is, we are not reading propagated info, but info straight from struct.news, which is precise.

Improving the propagation could be good as well, though it would not help this part of the optimization (since it already looks at precise data).

Copy link
Member

Choose a reason for hiding this comment

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

I mean that we should never even call optimizeUsingRefTest if there is only a single constant value when you take exactness into account. It would be best if the propagated info used by optimizeRead already accounted for exactness. Then we wouldn't have to complicate optimizeUsingRefTest by having it sometimes optimize not using RefTest.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see your point. However, I think the best course of action is to refactor this code so that the reftest code is used by both paths. That is, the reftest code has precise analysis already - we can just use that, without (harder) work to change the propagation. (And then we can rename it "the general path", with a flag "allow reftest")

Put another way, we propagate for the inexact case, and keep the raw data for the exact case. And we have a function that uses that data correctly right now, reftest. Changing the propagation would be more work.

I'd like to leave that as a followup, though, to keep this PR simple.

Copy link
Member

Choose a reason for hiding this comment

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

I don't want to delay getting this fix in, but I really do think making the propagation more precise is the right fix. Then no other future pass that also uses struct-utils.h will have to special-case exactness to avoid missing an optimization.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not opposed to precise propagation, but it's a separate issue from this PR. Propagation is done so that we can easily notice the effects of subtypes. But when we have exact info, we can just read that info, that we do have it here - rawNewInfos as I mentioned. So the only question is where to read that info. The simplest thing seemed to be to use it in the reftest logic, so I started with that.

But, it might be clearer to just use exactness earlier, not just in the reftest logic. Might be faster too. I pushed a commit with that now. It's late on Friday so I'm not sure I got it right... I'll check on Monday.

}
return;
}

Expand Down
88 changes: 88 additions & 0 deletions test/lit/passes/cfp-reftest-desc.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,3 +106,91 @@
)
)

(module
(rec
;; CHECK: (rec
;; CHECK-NEXT: (type $super (sub (descriptor $super.desc (struct))))
(type $super (sub (descriptor $super.desc (struct))))
;; CHECK: (type $super.desc (sub (describes $super (struct (field (ref (exact $func)))))))
(type $super.desc (sub (describes $super (struct (field (ref (exact $func)))))))
Copy link
Member

Choose a reason for hiding this comment

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

What purpose does the funcref field serve? Can it be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, removed.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it's still there. Forgot to push?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, yes, pushed now.


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

;; CHECK: (type $sub (sub $super (descriptor $sub.desc (struct))))
(type $sub (sub $super (descriptor $sub.desc (struct))))
;; CHECK: (type $sub.desc (sub $super.desc (describes $sub (struct (field (ref (exact $func)))))))
(type $sub.desc (sub $super.desc (describes $sub (struct (field (ref (exact $func)))))))
)

;; CHECK: (type $5 (func (result (ref (exact $super.desc)))))

;; CHECK: (global $A (ref (exact $super.desc)) (struct.new $super.desc
;; CHECK-NEXT: (ref.func $func)
;; CHECK-NEXT: ))
(global $A (ref (exact $super.desc)) (struct.new $super.desc
(ref.func $func)
))

;; CHECK: (global $B (ref (exact $sub.desc)) (struct.new $sub.desc
;; CHECK-NEXT: (ref.func $func)
;; CHECK-NEXT: ))
(global $B (ref (exact $sub.desc)) (struct.new $sub.desc
(ref.func $func)
))

;; CHECK: (func $test (type $5) (result (ref (exact $super.desc)))
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.new_default $super
;; CHECK-NEXT: (global.get $A)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.new_default $sub
;; CHECK-NEXT: (global.get $B)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (block (result (ref (exact $super.desc)))
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (ref.as_non_null
;; CHECK-NEXT: (block (result nullref)
;; CHECK-NEXT: (ref.null none)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (global.get $A)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $test (result (ref (exact $super.desc)))
(drop
(struct.new_default $super
(global.get $A)
)
)
(drop
(struct.new_default $sub
(global.get $B)
)
)
;; We read from an exact $super here, so the type of the ref.get_desc is
;; exact as well. If we ignore that in the optimization, we might think that
;; the two struct.news before us are two possible values, one from $super and
;; one from $sub, and if we emitted a ref.test between those values, we'd get
;; a non-exact value that does not validate.
;;
;; Instead, we should look only at $super itself, and optimize to $A.
(ref.get_desc $super
(block (result (ref null (exact $super)))
(ref.null $super)
)
)
)

;; CHECK: (func $func (type $func) (param $0 i32) (result i32)
;; CHECK-NEXT: (i32.const 42)
;; CHECK-NEXT: )
(func $func (type $func) (param $0 i32) (result i32)
(i32.const 42)
)
)

55 changes: 55 additions & 0 deletions test/lit/passes/cfp-reftest.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1456,3 +1456,58 @@
)
)
)

(module
;; CHECK: (type $struct (sub (struct (field i32))))
(type $struct (sub (struct i32)))
;; CHECK: (type $1 (func))

;; CHECK: (type $substruct (sub $struct (struct (field i32) (field f64))))
(type $substruct (sub $struct (struct i32 f64)))

;; CHECK: (type $3 (func (param (ref null (exact $struct))) (result i32)))

;; CHECK: (func $create (type $1)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.new $struct
;; CHECK-NEXT: (i32.const 10)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.new $substruct
;; CHECK-NEXT: (i32.const 20)
;; CHECK-NEXT: (f64.const 3.14159)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $create
;; Used below.
(drop
(struct.new $struct
(i32.const 10)
)
)
(drop
(struct.new $substruct
(i32.const 20)
(f64.const 3.14159)
)
)
)
;; CHECK: (func $get (type $3) (param $struct (ref null (exact $struct))) (result i32)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (ref.as_non_null
;; CHECK-NEXT: (local.get $struct)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (i32.const 10)
;; CHECK-NEXT: )
(func $get (param $struct (ref null (exact $struct))) (result i32)
;; The type here is exact, so we do not even need to do a select: only the
;; super's value is possible, 10.
(struct.get $struct 0
(local.get $struct)
)
)
)

Loading