-
Notifications
You must be signed in to change notification settings - Fork 745
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
base: main
Are you sure you want to change the base?
Conversation
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? |
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 |
But would have done anything at all? Can we just let wasm2js emit worse code? |
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. |
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 likeHEAP32[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 anunreachable
will be removed, but that is not what wasm2js needs: it just wants to marki32.load
etc. as not trapping (but change nothing aboutunreachable
).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 thattargetsJS
means thati32.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.