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

Introduce compiler-rt and add bitwise and, or, xor polyfills #89

Merged
merged 6 commits into from
Nov 19, 2024

Conversation

wzmuda
Copy link
Contributor

@wzmuda wzmuda commented Nov 14, 2024

Summary

Introduce compiler runtime module. Add relevant CI changes and update the PR template to reflect that we should also format the Cairo code.

Start implementing ALU. Add the top-level module and submodules for and, or and xor operations. Add relevant readmes.

Implemented polyfills:

and

Implements bitwise and operation.

API

  • __llvm_and_bool_bool(lhs: u128, rhs: u128) -> u128
  • __llvm_and_i8_i8(lhs: u128, rhs: u128) -> u128
  • __llvm_and_i16_i16(lhs: u128, rhs: u128) -> u128
  • __llvm_and_i32_i32(lhs: u128, rhs: u128) -> u128
  • __llvm_and_i64_i64(lhs: u128, rhs: u128) -> u128
  • __llvm_and_i128_i128(lhs: u128, rhs: u128) -> u128

or

Implements bitwise or operation.

API

  • __llvm_or_bool_bool(lhs: u128, rhs: u128) -> u128
  • __llvm_or_i8_i8(lhs: u128, rhs: u128) -> u128
  • __llvm_or_i16_i16(lhs: u128, rhs: u128) -> u128
  • __llvm_or_i32_i32(lhs: u128, rhs: u128) -> u128
  • __llvm_or_i64_i64(lhs: u128, rhs: u128) -> u128
  • __llvm_or_i128_i128(lhs: u128, rhs: u128) -> u128

xor

Implements bitwise xor operation.

API

  • __llvm_xor_bool_bool(lhs: u128, rhs: u128) -> u128
  • __llvm_xor_i8_i8(lhs: u128, rhs: u128) -> u128
  • __llvm_xor_i16_i16(lhs: u128, rhs: u128) -> u128
  • __llvm_xor_i32_i32(lhs: u128, rhs: u128) -> u128
  • __llvm_xor_i64_i64(lhs: u128, rhs: u128) -> u128
  • __llvm_xor_i128_i128(lhs: u128, rhs: u128) -> u128

Other PRs for issue #38 will follow.

Details

  • Are we sure about using u128 instead of felts?
  • Does my testing approach make sense? I generated all these cases with Python.
  • Apart from code please rate the overall code organization.

Checklist

  • Code is formatted by Rustfmt or scarb fmt.
  • Documentation has been updated if necessary.

Copy link
Collaborator

@iamrecursion iamrecursion left a comment

Choose a reason for hiding this comment

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

A good start, but definitely needs some changes in my view. One thing I want to see is documentation of the structure and why this structure is chosen.

compiler_rt/src/alu.cairo Outdated Show resolved Hide resolved
compiler_rt/src/alu/test_cases.cairo Outdated Show resolved Hide resolved
compiler_rt/src/alu/and_bool.cairo Outdated Show resolved Hide resolved
compiler_rt/src/alu/and_i128.cairo Outdated Show resolved Hide resolved
@wzmuda wzmuda force-pushed the wz/alu-bit-ops branch 2 times, most recently from eac1dc7 to 06124cc Compare November 15, 2024 23:52
@wzmuda
Copy link
Contributor Author

wzmuda commented Nov 15, 2024

One thing I want to see is documentation of the structure and why this structure is chosen.

Sure, where would be a good place for this? Dedicated README.md in the compiler_rt directory perhaps? That would be a central point for everybody who starts reading/modifying the runtime and would display nicely on github.

@wzmuda wzmuda force-pushed the wz/alu-bit-ops branch 5 times, most recently from cca754f to 106000a Compare November 17, 2024 22:04
@wzmuda wzmuda force-pushed the wz/alu-bit-ops branch 2 times, most recently from 366eb21 to bb94e00 Compare November 17, 2024 23:41
@wzmuda wzmuda marked this pull request as ready for review November 17, 2024 23:41
@wzmuda wzmuda requested a review from a team as a code owner November 17, 2024 23:41
@wzmuda wzmuda changed the title Introduce compiler_rt and add bitwise and polyfills Introduce compiler_rt and add bitwise and, or, xor polyfills Nov 18, 2024
Copy link
Collaborator

@iamrecursion iamrecursion left a comment

Choose a reason for hiding this comment

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

Good stuff, but still more to do! Please apply comments I have made on one polyfill (or set of polyfills) to all relevant polyfills; I didn't want to repeat myself too much.

.github/workflows/scarb.yml Outdated Show resolved Hide resolved
.github/workflows/scarb.yml Outdated Show resolved Hide resolved
compiler_rt/README.md Outdated Show resolved Hide resolved
compiler_rt/README.md Outdated Show resolved Hide resolved
compiler_rt/README.md Outdated Show resolved Hide resolved
compiler_rt/src/alu/and/and_bool.cairo Outdated Show resolved Hide resolved
compiler_rt/src/alu/and/and_i128.cairo Outdated Show resolved Hide resolved
compiler_rt/src/alu/and/and_i16.cairo Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@wzmuda wzmuda force-pushed the wz/alu-bit-ops branch 2 times, most recently from 793118a to 3e0facf Compare November 18, 2024 17:46
Copy link
Collaborator

@iamrecursion iamrecursion left a comment

Choose a reason for hiding this comment

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

Good stuff, but there is yet more to do!

compiler_rt/README.md Outdated Show resolved Hide resolved
compiler_rt/src/alu/and/and_bool.cairo Outdated Show resolved Hide resolved
.github/workflows/scarb.yml Outdated Show resolved Hide resolved
compiler_rt/src/alu/and/and_i1.cairo Outdated Show resolved Hide resolved
Run `npm audit fix`. This was motivated by the following error:
        [error] Cannot find package 'prettier-plugin-toml' imported from /Users/wz/reilabs/starknet/hieratika/noop.js

Signed-off-by: Wojciech Zmuda <[email protected]>
Signed-off-by: Wojciech Zmuda <[email protected]>
@wzmuda wzmuda force-pushed the wz/alu-bit-ops branch 2 times, most recently from 85051eb to ad25010 Compare November 18, 2024 23:35
Copy link
Collaborator

@iamrecursion iamrecursion left a comment

Choose a reason for hiding this comment

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

LGTM! One last question; can we have the directory called compiler-rt even if the project is compiler_rt? It would be cleaner.

one of the polyfills.

This project's name deliberately resembles [LLVM's `compiler-rt`](https://compiler-rt.llvm.org/),
because it served the same purpose of providing implementations of the low-level operations written
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
because it served the same purpose of providing implementations of the low-level operations written
because it serves the same purpose of providing implementations of the low-level operations written

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

Add skeleton for the compiler_rt project, which is a scarb project
containing Cairo code implementing Hieratika polyfills.

The first added operations are:
- `__llvm_and_i1_i1`
- `__llvm_and_i8_i8`
- `__llvm_and_i16_i16`
- `__llvm_and_i32_i32`
- `__llvm_and_i64_i64`
- `__llvm_and_i128_i128`

Signed-off-by: Wojciech Zmuda <[email protected]>
This workflow tests and formats Cairo code. It is triggered only on
changes done to the compiler-rt directory.

Signed-off-by: Wojciech Zmuda <[email protected]>
Add `__llvm_or_iN_iN` family of polyfills to ALU.

Signed-off-by: Wojciech Zmuda <[email protected]>
Add `__llvm_xor_iN_iN` family of polyfills to ALU.

Signed-off-by: Wojciech Zmuda <[email protected]>
@wzmuda wzmuda changed the title Introduce compiler_rt and add bitwise and, or, xor polyfills Introduce compiler-rt and add bitwise and, or, xor polyfills Nov 19, 2024
@wzmuda wzmuda merged commit 7b7da08 into main Nov 19, 2024
10 checks passed
@wzmuda wzmuda deleted the wz/alu-bit-ops branch November 19, 2024 19:54
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