Skip to content

Use 3 modes for potentially trapping ops in asm2wasm#929

Merged
kripken merged 4 commits intomasterfrom
imprecise-float
Mar 7, 2017
Merged

Use 3 modes for potentially trapping ops in asm2wasm#929
kripken merged 4 commits intomasterfrom
imprecise-float

Conversation

@kripken
Copy link
Copy Markdown
Member

@kripken kripken commented Mar 1, 2017

  • allow (just emit a potentially trapping op)
  • js (do exactly what js does, even if it takes a slow ffi to do it)
  • clamp (avoid the trap by clamping as necessary)

See emscripten-core/emscripten#4625

…t a potentially trapping op), js (do exactly what js does, even if it takes a slow ffi to do it), and clamp (avoid the trap by clamping as necessary)
@sunfishcode
Copy link
Copy Markdown
Member

This looks good to me. Obviously we could micro-optimize some things more if we really wanted, and the choice of what mode to have by default depends on how one weights various criteria, but what's here is a reasonable step forward.

@dschuff
Copy link
Copy Markdown
Member

dschuff commented Mar 6, 2017

I'm on board with this general idea too. A few comments, and questions to make sure I understand right:

  1. It looks like this mode applies to both float->int and div/rem (I'm happy with that, although if there were some good reason to have separate flags/modes, I'd be ok with that in this tool too since it's not an emscripten-user-facing tool).
  2. As a nit, names like potentialTrapMode/potentiallyTrappingFoo seem unnecessarily verbose; I think the "potential" part goes without saying (we already have an op that always traps :) ) trappingOpMode (or even just trapMode) and trappingDiv/trappingConversion etc seem more readable.
    but yeah in general this looks good to me too.

@dschuff
Copy link
Copy Markdown
Member

dschuff commented Mar 6, 2017

Actually also just so I understand. It looks like the clamp and JS mode for div/rem do the same thing now? Just return 0?

@kripken
Copy link
Copy Markdown
Member Author

kripken commented Mar 6, 2017

  1. Yeah, this applies to both. We may split them up eventually, but actually I am thinking that is less likely, since we now have very fast non-trapping i32 operations (no ffis).
  2. Maybe a little verbose, I agree. We already had makePotentiallyTrappingI32Binary etc., and it seems hard to shorten that (I think we discussed makeUnsafe* at some point, but not clear enough, and makeTrapping* is not accurate), so PotentialTrapMode seems consistent with that.
  3. Yeah, clamp and JS mode do the same for int div/rem, both "clamp" div/rem of 0 to 0, the JS default here is good.

@dschuff
Copy link
Copy Markdown
Member

dschuff commented Mar 6, 2017

Actually I think makeTrapping* is accurate. Just because something is "trapping" doesn't mean it necessarily always traps, just that it may trap.

@kripken
Copy link
Copy Markdown
Member Author

kripken commented Mar 6, 2017

Ok, not my first intuition but I can see how that works too, and shorter is nicer.

Comment thread src/asm2wasm.h
input = conv;
}
// We can handle this in one of two ways: clamping, which is fast, or JS, which
// is precisely like JS but in order to do that we do a slow ffi
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This behavior is fine for now; I'd be very interested in seeing how wasm build of __fixsfsi or __fixdfdi compares with FFI+JS version.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, worth trying that.

@kripken
Copy link
Copy Markdown
Member Author

kripken commented Mar 7, 2017

Ok, how does this look now with the shorter names?

@kripken kripken merged commit 71804e2 into master Mar 7, 2017
@kripken kripken deleted the imprecise-float branch March 7, 2017 21:56
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