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.

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

RemoveUnusedModuleElements: Track CallRef/RefFunc more precisely #4621

Merged
merged 12 commits into from
Apr 28, 2022
Merged

Conversation

kripken
Copy link
Member

@kripken kripken commented Apr 27, 2022

If we see (ref.func $foo) that does not mean that $foo is reachable - we
must also see a (call_ref ..) of the proper type. Only after seeing both should
we mark the function as reachable, which this PR does.

This adds some complexity as we need to track intermediate state as we go,
since we could see the RefFunc before the CallRef or vice versa. We also
need to handle the case of a RefFunc without a CallRef properly: We cannot
remove the function, as the RefFunc must refer to it, but at least we can
empty out the body since we know it is never reached.

This removes an old wasm-opt test which is now superseded by a new lit
test.

On J2Wasm output this removes 3% of all functions, which account for
2.5% of total code size.

@kripken kripken requested review from tlively and aheejin April 27, 2022 17:30
Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

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

This does not handle a case of a signature whose call_refs only exist in to-be-removed functions, right? For example, an unexported function $a's signature is vii and it's only call_refs are within func $a. I guess handling this case can be nontrivial and might not bring much benefit anyway, but just wanted to check.

Comment on lines +271 to +272
// RefFuncs that are never called are a special case: We cannot remove the
// function, since then (ref.func $foo) would not validate. But if we know
Copy link
Member

Choose a reason for hiding this comment

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

Can't we use (ref.null func)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I don't think so. The type of a null is nullable while (ref.func $foo) is non-nullable. Also, someone might do ref.is_null on the ref, even if the ref is never called.

@kripken
Copy link
Member Author

kripken commented Apr 28, 2022

This does not handle a case of a signature whose call_refs only exist in to-be-removed functions, right?

If I understand your example, I think that case is handled. This pass starts from the reachable roots and goes from there, so anything we scan is reachable. A to-be-removed function would never be scanned, so we'd never see the RefFuncs or the CallRefs in it.

@kripken kripken merged commit 408b2eb into main Apr 28, 2022
@kripken kripken deleted the rume branch April 28, 2022 18:26
kripken added a commit that referenced this pull request May 2, 2022
We assume a closed world atm in the GC space, but the call.without.effects
intrinsic sort of breaks that: that intrinsic looks like an import, but we really
need to care about what is sent to it even in a closed world:

(call $call-without-effects
  (ref.func $target-keep)
)

That reference cannot be ignored, as logically it is called just as if there
were a call_ref there. This adds support for that, fixing the combination of
#4621 and using call.without.effects.

Also flip the vector of ref.func names to a set. I realized that in a very
large program we might see the same name many times.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants