Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Dec 13, 2024

We use __builtin_rint (which lowers to a single f64.nearest
instruction) when compiling the musl version of rint.c so there there is
no need to use a JS call for this case.

@sbc100 sbc100 requested a review from kripken December 13, 2024 01:00
@sbc100 sbc100 changed the title Remove round and rint from jsmath library. NFC Remove rint from jsmath library. NFC Dec 13, 2024
@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 13, 2024

I updated this change to only effect rint which is clearly not necessary.

@sbc100 sbc100 requested a review from kripken December 13, 2024 19:46
We also use `__builtin_rint` (which lowers to a single f64.nearest
instruction) when compiling the musl version of rint.c so there there is
no need to use a JS call for this case.
return x >= 0 ? Math.floor(x + 0.5) : Math.ceil(x - 0.5);
}
return (x - Math.floor(x) != .5) ? round(x) : round(x / 2) * 2;
})
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I am not certain the new code is smaller, as unless I am doing something wrong, even though rint.c just does __builtin_rint, it doesn't turn into a single instruction. My testcase:

#include <emscripten.h>

int main(int argc, char **argv) {
  double x = double(argc + 1000) / double(argc + 100);
  return __builtin_rint(x);
}

emcc -O3 --profiling leads to

 (func $main (param $0 i32) (param $1 i32) (result i32)
  (local $2 f64)
  (if
   (f64.lt
    (f64.abs
     (local.tee $2
      (f64.nearest
       (f64.div
        (f64.convert_i32_s
         (i32.add
          (local.get $0)
          (i32.const 1000)
         )
        )
        (f64.convert_i32_s
         (i32.add
          (local.get $0)
          (i32.const 100)
         )
        )
       )
      )
     )
    )
    (f64.const 2147483648)
   )
   (then
    (return
     (i32.trunc_f64_s
      (local.get $2)
     )
    )
   )
  )
  (i32.const -2147483648)
 )

Ignoring the division etc., it looks like rint is not a single instruction but f64.nearest, abs, lt, trunc operations?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

rint does compile to single instruction:

$ ar x ./cache/sysroot/lib/wasm64-emscripten/libc.a rint.o
$ wasm-objdump -d -r -x rint.o
...
000059 func[0] <rint>:
 00005a: 20 00                      | local.get 0
 00005c: 9e                         | f64.nearest
 00005d: 0b                         | end

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The rest of the complixity in your example I think comes from the conversion from double to int because your are returning and double from your main function.

If I do this I see the expected results:

$ cat test.c
extern double x;

int main(int argc, char **argv) {
  x = __builtin_rint(x);
}
$ ./emcc -Os test.c -c
$ dump test.o
000043 func[0] <__main_argc_argv>:
 000044: 41 00                      | i32.const 0
 000046: 41 00                      | i32.const 0
 000048: 2b 03 80 80 80 80 00       | f64.load 3 0
           00004a: R_WASM_MEMORY_ADDR_LEB 1 <x>
 00004f: 9e                         | f64.nearest
 000050: 39 03 80 80 80 80 00       | f64.store 3 0
           000052: R_WASM_MEMORY_ADDR_LEB 1 <x>
 000057: 41 00                      | i32.const 0
 000059: 0b                         | end

Copy link
Member

Choose a reason for hiding this comment

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

Oh, you're right, I missed the other conversion, sorry...

@sbc100 sbc100 enabled auto-merge (squash) December 13, 2024 20:40
@sbc100 sbc100 disabled auto-merge December 13, 2024 22:44
@sbc100 sbc100 merged commit ebd6f3c into emscripten-core:main Dec 13, 2024
26 of 28 checks passed
@sbc100 sbc100 deleted the simplify_jsmath branch December 13, 2024 22:44
hedwigz pushed a commit to hedwigz/emscripten that referenced this pull request Dec 18, 2024
We  use `__builtin_rint` (which lowers to a single `f64.nearest`
instruction) when compiling the musl version of `rint.c` so there there
is
no need to use a JS call for this case.
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.

2 participants