wasm2asm i32 arithmetic support#1120
Conversation
Updates CMakeLists.txt to have wasm2asm built by default, updates wasm2asm.h to account for recent interface changes, and restores JSPrinter functionality.
|
|
||
| Function* makePopcntFunc(MixedArena& allocator, bool is32Bit=true) { | ||
| Builder b(allocator); | ||
| // int c; for (c = 0; x != 0; c++) { x = x & (x - 1) }; return c |
There was a problem hiding this comment.
Oh I thought this was commented-out code at first
Probably worth a comment like "This is implemented as" to make that clearer.
| Ref Wasm2AsmBuilder::processWasm(Module* wasm) { | ||
| wasm->addFunction(makeCtzFunc(wasm->allocator)); | ||
| wasm->addFunction(makePopcntFunc(wasm->allocator)); | ||
| wasm->addFunction(makeRotFunc(wasm->allocator, true)); |
There was a problem hiding this comment.
Hm, bool parameters are kinda icky because they tend to not offer context at the call site for what true/false actually means. (https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/06/02/against-bool-parameters/)
How about makeRotFunc takes a WasmType instead of a bool, we assert(type == i32 || type == i64), and locally in makeRotFunc we have bool is32Bit = type == i32;
Edit: OH, this true/false is for left/right rotation. Maybe similar advice, but with opcodes instead of types? This actually lets you collapse the two bools, because RotLInt32 encodes all the information you need.
| } | ||
|
|
||
| Ref Wasm2AsmBuilder::processWasm(Module* wasm) { | ||
| wasm->addFunction(makeCtzFunc(wasm->allocator)); |
There was a problem hiding this comment.
I'd put these addFunction calls into their own method, addWasmCompatibilityFunctions or something. The list of functions to add is likely to grow, and it's logically one operation.
There was a problem hiding this comment.
I was also thinking of lazily adding these functions as they're required. Do you think that would be worth doing?
| static BinaryOp shifters[2][2] = {{ShrUInt64, ShrUInt32}, | ||
| {ShlInt64, ShlInt32}}; | ||
| Name funcName = names[isLRot][is32Bit]; | ||
| BinaryOp lshift = shifters[isLRot][is32Bit]; |
There was a problem hiding this comment.
Kind of interesting that lshift can be a right shift when !isLRot. That's probably fine, but a bit odd.
| UnaryOp clzOp = is32Bit ? ClzInt32 : ClzInt64; | ||
| UnaryOp eqzOp = is32Bit ? EqZInt32 : EqZInt64; | ||
| WasmType argType = is32Bit ? i32 : i64; | ||
| Binary* xorExp = b.makeBinary(xorOp, b.makeGetLocal(0, i32), |
There was a problem hiding this comment.
the convention in other code is
b.makeBinary(
xorOp,
b.makeGetLocal(0, i32),
b.makeBinary(
subOp,
b.makeGetLocal(0, i32),
b.makeConst(Literal(1))
)
);
(i.e. always nest, and always 2 spaces for nesting)
also, when creating a literal, we normally use an explicit type, Literal(int32_t(1)) so there is no ambiguity. however, i suppose there is no ambiguity for int32 constants, even on 64-bit, is that right?
|
|
||
| MATH_MIN("Math_min"), | ||
| MATH_MAX("Math_max"), | ||
| CTZ32("__ctz_i32"), |
There was a problem hiding this comment.
perhaps these should be prefixed? maybe wasm_ctz_i32 etc.?
3 participants
Injects implementations of rotl, rotr, ctz, and popcnt instructions into modules as separate functions.