Skip to content

Improved 64 bit addition #139

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 0 commits into from
Closed

Improved 64 bit addition #139

wants to merge 0 commits into from

Conversation

copy
Copy link
Contributor

@copy copy commented Mar 3, 2016

I added some tests, how can I run them?

@bobzhang
Copy link
Member

bobzhang commented Mar 3, 2016

thanks! are you familiar with nodejs?
see package.json in the project directory

cd ./jscom/test && make all
npm install --save-dev
npm test

({lo = other_low_; hi = other_high_} : t) =
let low = logand (add this_low_ other_low_) 0xFFFFFFFFn in
let overflow = if other_low_ < 1n
then sub Nativeint.min_int other_low_ < this_low_
Copy link
Member

Choose a reason for hiding this comment

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

what's your expected min_int and max_int here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They should be -0x80000000 and 0x7fffffff. It's probably not correct for bucklescript's Nativeint (depending on how we implement it).

@copy
Copy link
Contributor Author

copy commented Mar 3, 2016

Running make all fails:

bucklescript/jscomp/test% make all
../bin/osc -I ../stdlib -I ../runtime -I ../lib  -safe-string -w -40  -c ../stdlib/pervasives.ml
File "command line", line 1:
Error: Unbound module Pervasives
Makefile:31: recipe for target '../stdlib/pervasives.cmj' failed
make: *** [../stdlib/pervasives.cmj] Error 2

@bobzhang
Copy link
Member

bobzhang commented Mar 3, 2016

you also need

cd ./runtime && make all && cd ..
cd ./stdlib && make all && cd ..
cd ./test && make all && cd ..

you can hard code min_int and max_int for now

@copy
Copy link
Contributor Author

copy commented Mar 3, 2016

Still having some issues:

bucklescript/jscomp/runtime% make all
../bin/osc  -w -40 -safe-string  -c caml_oo.mli
File "command line", line 1:
Error: Unbound module Pervasives
Makefile:22: recipe for target 'caml_oo.cmi' failed
make: *** [caml_oo.cmi] Error 2
bucklescript/jscomp/test% make all
../bin/osc -I ../stdlib -I ../runtime -I ../lib  -safe-string -w -40  -c ../runtime/caml_string.ml
File "../runtime/caml_string.ml", line 1:
Error: Could not find the .cmi file for interface ../runtime/caml_string.mli.
Makefile:31: recipe for target '../runtime/caml_string.cmj' failed
make: *** [../runtime/caml_string.cmj] Error 2

@bobzhang
Copy link
Member

bobzhang commented Mar 3, 2016

Hi I made a fresh copy of your repo and it works for me, looking at the error messages, did you relocate your patched compiler later?

bgit>git clone https://github.com/bloomberg/bucklescript copy                                                                                                                 
Cloning into 'copy'...                                                                                                                                                        
remote: Counting objects: 5756, done.                                                                                                                                         
remote: Compressing objects: 100% (267/267), done.                                                                                                                            
remote: Total 5756 (delta 95), reused 0 (delta 0), pack-reused 5489                                                                                                           
Receiving objects: 100% (5756/5756), 16.32 MiB | 5.24 MiB/s, done.                                                                                                            
Resolving deltas: 100% (3610/3610), done.                                                                                                                                     
Checking connectivity... done.                                                                                                                                                
Checking out files: 100% (1193/1193), done.                                                                                                                                   
bgit>cd copy/jscomp/                                                                                                                                                          
jscomp>ocamlopt.opt -g -inline 100 -linkall  -I +compiler-libs -I bin ocamlcommon.cmxa ocamlbytecomp.cmxa bin/compiler.mli bin/compiler.ml -o bin/osc                         
jscomp>cd runtime                                                                                                                                                             
runtime>make all                                                                                                                                                              
../bin/osc  -w -40 -safe-string  -c caml_oo.mli                                                                                                                               
../bin/osc  -w -40 -safe-string  -c caml_oo.ml                                                                                                                                
../bin/osc  -w -40 -safe-string  -c caml_curry.ml                                                                                                                             
../bin/osc  -w -40 -safe-string  -c caml_array.mli                                                                                                                            
../bin/osc  -w -40 -safe-string  -c caml_array.ml                                                                                                                             
../bin/osc  -w -40 -safe-string  -c caml_string.mli                                                                                                                           
../bin/osc  -w -40 -safe-string  -c caml_string.ml                                                                                                                            
../bin/osc  -w -40 -safe-string  -c caml_exceptions.mli                                                                                                                       
../bin/osc  -w -40 -safe-string  -c caml_exceptions.ml                                                                                                                        
../bin/osc  -w -40 -safe-string  -c caml_obj.mli                                                                                                                              
../bin/osc  -w -40 -safe-string  -c caml_obj.ml                                                                                                                               
../bin/osc  -w -40 -safe-string  -c caml_int64.ml 

@bobzhang
Copy link
Member

bobzhang commented Mar 3, 2016

btw, after I replaced Native.min_int with -0x8000_0000n and Nativeint.max_int with 0xffff_ffffn, and runt tests(assume you have mocha installed). Actually you have to do the hard encoding, since runtime is a low dependency, it can not depend on stdlib, Native.shift_* is okay since it's a primitive and inlined

mocha test/*test.js
192 passing (350ms)

It looks good

@bobzhang
Copy link
Member

bobzhang commented Mar 3, 2016

btw, another interesting encoding is new Int32Array(2)

@copy
Copy link
Contributor Author

copy commented Mar 4, 2016

btw, another interesting encoding is new Int32Array(2)

That'd be great, especially if it could be applied to all records containing only ints (char and int32 fit too).

did you relocate your patched compiler later?

Yep, that was it. I can run the tests now. However, they fail for me. Actually there seems to be a bug in the overflow check. I'll try to fix it.

@copy
Copy link
Contributor Author

copy commented Mar 4, 2016

Should be good now

@bobzhang
Copy link
Member

bobzhang commented Mar 4, 2016

Cool, for your info, I used watchman to build those ml files automatically, (see build.json)

watchman -j < build.json # every time you change ml files regen js file
mocha -w -R list test/*test.js # watch mode for mocha, every time js files changed re-run the test

I will check with our legal department before merging your changes, it should not take that long, thanks for contribution !

@bobzhang
Copy link
Member

bobzhang commented Mar 4, 2016

For your info, I fixed a bug in the compiler so that I also added functionality of int64 mul (it's pretty naive, literally translated from google closure library)

@bobzhang
Copy link
Member

bobzhang commented Mar 8, 2016

@copy Just let you know that @kpfleming is working on the contribution page, we appreciate your work and will merge it once we have a contribution guide line, thanks again.

@bobzhang
Copy link
Member

bobzhang commented Mar 9, 2016

hi @copy, we have a file about how to contribute(https://github.com/bloomberg/bucklescript/blob/master/CONTRIBUTING.md), would you rebase your change when you have time? thanks

@copy copy closed this Mar 10, 2016
EduardoRFS pushed a commit to EduardoRFS/bucklescript that referenced this pull request Apr 6, 2021
kevinbarabash pushed a commit to kevinbarabash/rescript-compiler that referenced this pull request Dec 24, 2021
…rescript-lang#139)

* implement syntax for arity zero vs arity one in uncurried application

Since there is no syntax space for arity zero vs arity one,
we parse
  `fn(. ())` into
  `fn(. {let __res_unit = (); __res_unit})`
  when the parsetree is intended for type checking

`fn(.)` is treated as zero arity application

* add CHANGELOG entry
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