Skip to content
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

Add bultins: i32/i64/f32/f64.add/sub/mul #1484

Merged
merged 17 commits into from
Oct 5, 2020
Merged

Add bultins: i32/i64/f32/f64.add/sub/mul #1484

merged 17 commits into from
Oct 5, 2020

Conversation

ValeriaVG
Copy link
Contributor

@ValeriaVG ValeriaVG commented Oct 3, 2020

  • add

  • sub

  • mul

  • Added i32/i64/f32/f64.add bultins

  • Added related types

  • Added tests for it to tests/compiler/builtins.ts and rebuilt related fixtures

  • Fixed one typo in tests/compiler/builtins.ts

Related issue: #1310

  • I've read the contributing guidelines

src/builtins.ts Outdated Show resolved Hide resolved
src/builtins.ts Outdated Show resolved Hide resolved
src/builtins.ts Outdated Show resolved Hide resolved
Copy link
Member

@MaxGraey MaxGraey left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Thanks!

@MaxGraey MaxGraey requested a review from dcodeIO October 3, 2020 12:24
ValeriaVG and others added 2 commits October 3, 2020 14:24
Co-authored-by: Max Graey <maxgraey@gmail.com>
Co-authored-by: Max Graey <maxgraey@gmail.com>
Copy link
Member

@dcodeIO dcodeIO left a comment

Choose a reason for hiding this comment

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

Thanks for looking into these! 👍 LGTM with the comment below resolved.

Do you plan to add sub, mul, div etc. as well?

std/assembly/index.d.ts Show resolved Hide resolved
@ValeriaVG
Copy link
Contributor Author

@dcodeIO Thank you.
Sure, I don't mind adding those. Would you like me to do it in this PR or a separate one?

@dcodeIO
Copy link
Member

dcodeIO commented Oct 3, 2020

I'd be fine with including similar ones in this PR, like the mentioned sub and mul. I think div and rem are a little different due to having both signed and unsigned variants so might make a good follow-up. Also going to update the original issue with a full list :)

@ValeriaVG ValeriaVG changed the title Add bultins: i32/i64/f32/f64.add Add bultins: i32/i64/f32/f64.add/sub/mul Oct 3, 2020
@ValeriaVG
Copy link
Contributor Author

I've added sub and mul in a similar manner, but I'm having an issue.
When I'm trying to assert the result in the tests like that:

i = sub<i32>(2, 1); assert(i == 1);

I'm getting this:

- instantiate untouched
  abort: null in (74:21)
  ---
  RuntimeError: unreachable
      at start:builtins (wasm://wasm/a0b0014e:0:1359)
      at ~start (wasm://wasm/a0b0014e:0:5573)

Any idea where to dig?

@MaxGraey
Copy link
Member

MaxGraey commented Oct 3, 2020

try to swap temp1 and temp2 here:

let ret =  module.binary(
   op,
   module.local_get(temp1.index, nativeType), // try temp2.index instead
   module.local_get(temp2.index, nativeType) // try temp1.index instead
);

I guess problem with wrong argument's order somewhere

src/builtins.ts Outdated Show resolved Hide resolved
src/builtins.ts Outdated Show resolved Hide resolved
src/builtins.ts Outdated Show resolved Hide resolved
src/builtins.ts Outdated Show resolved Hide resolved
src/builtins.ts Outdated Show resolved Hide resolved
src/builtins.ts Outdated Show resolved Hide resolved
src/builtins.ts Outdated Show resolved Hide resolved
ValeriaVG and others added 6 commits October 4, 2020 08:26
Co-authored-by: Max Graey <maxgraey@gmail.com>
Co-authored-by: Max Graey <maxgraey@gmail.com>
Co-authored-by: Max Graey <maxgraey@gmail.com>
Co-authored-by: Max Graey <maxgraey@gmail.com>
Co-authored-by: Max Graey <maxgraey@gmail.com>
Co-authored-by: Max Graey <maxgraey@gmail.com>
src/builtins.ts Outdated Show resolved Hide resolved
@dcodeIO dcodeIO merged commit 3223f76 into AssemblyScript:master Oct 5, 2020
@github-actions
Copy link

github-actions bot commented Oct 6, 2020

🎉 This PR is included in version 0.14.13 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants