WebAssembly
Remove IgnoreImplicitTraps [DO NOT LAND]
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.
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 effects.h). I'm not sure the benefit of removing IIT is worth that.
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.