implementation and tests for clz, ctz, and popcnt #50
Conversation
naturaltransformation
force-pushed
the
master
branch
2 times, most recently
from
c1c3450
to
2ed82ea
Compare
|
(drive-by confirmation that George is part of the W3C CG under his company, thanks for the contribution!) |
|
This contains code from the closed #7 that was dropped due to not being from a CG participant. Just to avoid any worries about that, this is to confirm that code was also from Intel and one of our WebAssembly CG reps has contributed it this time (I'm Intel's W3C AC rep). |
|
|
||
| (func $pcnt32 (param $x i32) (result i32) | ||
| (i32.popcnt (get_local $x)) | ||
| ) |
There was a problem hiding this comment.
It'd be nice to keep the function name identical to the opcode name.
There was a problem hiding this comment.
On 16 September 2015 at 01:36, Dan Gohman notifications@github.com wrote:
In ml-proto/test/int32.wasm
#50 (comment):@@ -0,0 +1,37 @@
+(; Int arith operations ;)
+
+(module
- (func $clz32 (param $x i32) (result i32)
- (i32.clz (get_local $x))
- )
- (func $ctz32 (param $x i32) (result i32)
- (i32.ctz (get_local $x))
- )
- (func $pcnt32 (param $x i32) (result i32)
- (i32.popcnt (get_local $x))
- )
It'd be nice to keep the function name identical to the opcode name.
Btw, despite it being what Intel picked on a bad day, popcnt really is an
odd name out. It is both inconsistent with the other count operators, and
it sounds like some kind of stack operator.
Would anybody mind if we changed it? For example, how about cnz (count
non-zeros), which would be in line with the other count operators. (If you
say it's a worthwhile idea I can create an issue or PR.)
There was a problem hiding this comment.
I may be wrong, but IIRC, the “population count” terminology long precedes Intel. It is used in GHC base package, GCC, Sparc, Cray, and CDC (the latter ones long before Intel). Even the Mark I had it but called it Sideways Add.
From: rossberg-chromium [mailto:notifications@github.com]
Sent: Wednesday, September 16, 2015 12:33 AM
To: WebAssembly/spec
Cc: Kuan, George
Subject: Re: [spec] implementation and tests for clz, ctz, and popcnt (#50)
In ml-proto/test/int32.wasmhttps://github.com//pull/50#discussion_r39600610:
@@ -0,0 +1,37 @@
+(; Int arith operations ;)
+(module
- (func $clz32 (param $x i32) (result i32)
- (i32.clz (get_local $x))
- )
- (func $ctz32 (param $x i32) (result i32)
- (i32.ctz (get_local $x))
- )
- (func $pcnt32 (param $x i32) (result i32)
- (i32.popcnt (get_local $x))
- )
On 16 September 2015 at 01:36, Dan Gohman notifications@github.com wrote: In ml-proto/test/int32.wasm implementation and tests for clz, ctz, and popcnt #50 (comment): > @@ -0,0 +1,37 @@ > +(; Int arith operations ;) > + > +(module > + (func $clz32 (param $x i32) (result i32) > + (i32.clz (get_local $x)) > + ) > + > + (func $ctz32 (param $x i32) (result i32) > + (i32.ctz (get_local $x)) > + ) > + > + (func $pcnt32 (param $x i32) (result i32) > + (i32.popcnt (get_local $x)) > + ) It'd be nice to keep the function name identical to the opcode name.
Btw, despite it being what Intel picked on a bad day,popcntreally is an odd name out. It is both inconsistent with the other count operators, and it sounds like some kind of stack operator. Would anybody mind if we changed it? For example, how aboutcnz(count non-zeros), which would be in line with the other count operators. (If you say it's a worthwhile idea I can create an issue or PR.)
—
Reply to this email directly or view it on GitHubhttps://github.com//pull/50/files#r39600610.
There was a problem hiding this comment.
On UltraSparc, it is known as POPC if that is any better.
From: rossberg-chromium [mailto:notifications@github.com]
Sent: Wednesday, September 16, 2015 12:33 AM
To: WebAssembly/spec
Cc: Kuan, George
Subject: Re: [spec] implementation and tests for clz, ctz, and popcnt (#50)
In ml-proto/test/int32.wasmhttps://github.com//pull/50#discussion_r39600610:
@@ -0,0 +1,37 @@
+(; Int arith operations ;)
+(module
- (func $clz32 (param $x i32) (result i32)
- (i32.clz (get_local $x))
- )
- (func $ctz32 (param $x i32) (result i32)
- (i32.ctz (get_local $x))
- )
- (func $pcnt32 (param $x i32) (result i32)
- (i32.popcnt (get_local $x))
- )
On 16 September 2015 at 01:36, Dan Gohman notifications@github.com wrote: In ml-proto/test/int32.wasm implementation and tests for clz, ctz, and popcnt #50 (comment): > @@ -0,0 +1,37 @@ > +(; Int arith operations ;) > + > +(module > + (func $clz32 (param $x i32) (result i32) > + (i32.clz (get_local $x)) > + ) > + > + (func $ctz32 (param $x i32) (result i32) > + (i32.ctz (get_local $x)) > + ) > + > + (func $pcnt32 (param $x i32) (result i32) > + (i32.popcnt (get_local $x)) > + ) It'd be nice to keep the function name identical to the opcode name.
Btw, despite it being what Intel picked on a bad day,popcntreally is an odd name out. It is both inconsistent with the other count operators, and it sounds like some kind of stack operator. Would anybody mind if we changed it? For example, how aboutcnz(count non-zeros), which would be in line with the other count operators. (If you say it's a worthwhile idea I can create an issue or PR.)
—
Reply to this email directly or view it on GitHubhttps://github.com//pull/50/files#r39600610.
naturaltransformation
force-pushed
the
master
branch
5 times, most recently
from
d73804d
to
a071fd8
Compare
| let rec helper n acc i = | ||
| match (n, i) with | ||
| | (n,_) when n = Int.zero -> acc | ||
| | (_,0) -> acc |
There was a problem hiding this comment.
Is this line ever reached? It's not clear that this line will ever match if the first line doesn't.
There was a problem hiding this comment.
The first line matches n=Int.zero and the second when i=0 (and n <> Int.zero). It would have been clearer and more succinct if this could be have turned into an OR pattern but alas OR patterns are incompatible with guards.
There was a problem hiding this comment.
From my reading of the code, when i=0, n must have been shifted down to zero too, which would mean this case isn't ever taken.
There was a problem hiding this comment.
Thanks! Actually, that is a great catch. I was using the wrong shift operator (it should be shift_right_logical instead of shift_right, now fixed). That case happened to mask the issue by handling negative integer inputs.
|
This code lgtm, with a few comments above. Eventually it'll be desirable to factor this code into functions outside of arithmetic.ml, but I think this is a good first step. Also, it'd be good for someone with more OCaml experience than I to review this. |
naturaltransformation
force-pushed
the
master
branch
4 times, most recently
from
a1cd68f
to
2aace69
Compare
| @@ -42,6 +43,9 @@ sig | |||
| val of_big_int_u : Big_int.big_int -> t | |||
| val to_value : t -> value | |||
| val of_value : int -> value -> t | |||
| val compare : t -> t -> int | |||
There was a problem hiding this comment.
Compare is polymorphic in Ocaml, you don't need to parameterise over it.
There was a problem hiding this comment.
Ok. But question though, does val compare : 'a -> 'a -> int vs. val compare: t -> t -> int make a difference to specialization either way?
There was a problem hiding this comment.
No, they are the same (would be rather terrible if they weren't :) ).
naturaltransformation
force-pushed
the
master
branch
from
2aace69
to
8b16080
Compare
|
Any further feedback on this one? Thanks! |
|
lgtm modulo nit |
naturaltransformation
force-pushed
the
master
branch
from
8b16080
to
1cd7b69
Compare
|
Merging with lgtm and nit addressed. Any further feedback can be addressed in followup patches. Thanks! |
implementation and tests for clz, ctz, and popcnt
* Clarify trapping semantics
* More minor opcode fixups Reserve opcode space for signed, and unsigned min/max, floating point RecipApprox, and RecipSqrtApprox and remove explicit `-` for reserved opcodes for consistency.
Update readme to link to the two active stack-switching proposals
Merge with `wasm-3.0`.
A short, illustrative implementation of integer bit operations, unoptimized since I assume the spec isn't prescribing particular optimizations. Test cases can benefit from hex integer literals though.