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

Remove IgnoreImplicitTraps [DO NOT LAND] #6647

Draft
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

kripken
Copy link
Member

@kripken kripken commented Jun 10, 2024

I experimented with removing IgnoreImplicitTraps but I don't think it is worth it. I'm uploading the code just to document the attempt.

IgnoreImplicitTraps marks e.g. i32.load as not having a trap effect. We later added TrapsNeverHappen which does a similar thing but in a more useful way, that says that if it traps then it is not reached in actual execution. The latter allows for use cases like "if condition then load", where the condition prevents the trap (for example, if a pointer is not null, then load from it: it would trap without the condition, but not with it). In practice IIT is very hard to enable on a real-world codebase (because of such conditions), while we have several real-world toolchains using TNH right now (Java, Dart). So it seems like it would be good to deprecate and remove the older option IIT.

However, IIT is used in wasm2js in a useful way, it turns out. In wasm2js an i32.load will never trap because we turn it into something like HEAP32[ptr >> 2] which is just a typed array read (which does not trap; if out of bounds it returns undefined, which will turn into 0 at the first coercion). Marking such loads as not trapping is quite useful for performance in wasm2js, so just turning off IIT would regress us. Replacing IIT with TNH does not quite work: TNH assumes traps are never reached, so for example code before an unreachable will be removed, but that is not what wasm2js needs: it just wants to mark i32.load etc. as not trapping (but change nothing about unreachable).

The code in this PR works around that by expanding the meaning of the targetsJS flag that we already have: In the PR we disable IIT (and do not enable TNH) and make the optimizer aware that targetsJS means that i32.load etc. do not trap. That works, but it offsets the code cleanups made possible by removing IIT - EffectAnalyzer loses one field but adds another, and as much code is added as is lost.

So overall it seems simpler to just keep IIT as it is, since wasm2js finds use in it.

@tlively
Copy link
Member

tlively commented Jun 10, 2024

Do we really need to continue supporting this just for wasm2js? If IIT didn't already exist, we certainly wouldn't add it just for wasm2js, right?

@kripken
Copy link
Member Author

kripken commented Jun 10, 2024

In that case we probably would've taken the shorter path, yeah, and not introduced IIT. But that would have led to worse code than what we have right now, actually (see the additional condition checks inside effects.h). I'm not sure the benefit of removing IIT is worth that.

@tlively
Copy link
Member

tlively commented Jun 10, 2024

But would have done anything at all? Can we just let wasm2js emit worse code?

@kripken
Copy link
Member Author

kripken commented Jun 10, 2024

Maybe in retrospect there would not have been enough pressure to optimize things, yeah. But it would be a noticeable regression if we went the other way now, which could be bad for users. Sometimes there is a strong reason for such a regression but I'm not sure that exists here.

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.

2 participants