Skip to content

Conversation

@kripken
Copy link
Member

@kripken kripken commented Dec 1, 2021

Previously this pass would see something like this and fail:

if (foo() + global) {
  global = 1;
}

The call to foo() has side effects, so we did not optimize. However, in such
a case the side effects are safe: they happen anyhow, regardless of the global
that we are optimizing. That is, "global" is read only to be written, even though
other things also influence the decision to write it. But "global" is not used in a
way that is observable: we can remove it, and nothing will notice (except for
things getting smaller/faster).

In other words, this PR will let us optimize the above example, while it also
needs to avoid optimizing the dangerous cases, like this:

if (foo(global)) {
  global = 1;
}

Here "global" flows into a place that notices its value and may use it aside
from deciding to write that global.

A common case where we want to optimize is combined ifs,

if (foo()) {
  if (global) {
    global = 1;
  }
}

which the optimizer turns into

if (foo() & global) {
  global = 1;
}

With this PR we can handle those things too.

This lets us optimize out some important globals in j2wasm like the initializer
boolean for the Math object, reducing some total 0.5% of code size.

@kripken kripken requested review from aheejin and tlively December 1, 2021 17:19
;; CHECK-NEXT: (if (result i32)
;; CHECK-NEXT: (call $foo)
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: (i32.const 0)
Copy link
Member

Choose a reason for hiding this comment

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

This i32.const 0 is chosen based on the global's initial value, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. The other parts of this pass recognize that that global has no more writes, so it is immutable. And then we propagate immutable global constants.

(i32.const 2)
)
(i32.add
(global.get $once)
Copy link
Member

Choose a reason for hiding this comment

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

It might be fun to sprinkle multiple reads of the global in here as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

That should work in principle, yeah. But it won't work atm, as we count the number of globals. We basically want to see that the number of reads of the global is equal to the number of places where it is "read but only to be written". Then we subtract the latter, which are the "safe" reads that we understand cause no other side effects than writing the global. If any other reads remain then we must assume the worst.

I do actually have a followup for this that I'll post once this lands, which now that I think of it may also handle that case, but I'll need to check.

@kripken kripken merged commit dc432f3 into main Dec 2, 2021
@kripken kripken deleted the once3 branch December 2, 2021 17:33
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