Add Bytes31 return type #610

pefontana · 2024-05-17T20:50:44Z

Add Bytes31 return type

Checklist

Linked to Github Issue
Unit tests added
Integration tests added.
This change requires new documentation.
- Documentation has been added/updated.

github-actions · 2024-05-17T21:23:25Z

Benchmarking results

Benchmark for program `factorial_2M`

Open benchmarks

Command	Mean [s]	Min [s]	Max [s]	Relative
`Cairo-vm (Rust, Cairo 1)`	11.431 ± 0.070	11.340	11.580	23.78 ± 0.17
`cairo-native (embedded AOT)`	1.532 ± 0.011	1.515	1.549	3.19 ± 0.03
`cairo-native (embedded JIT using LLVM's ORC Engine)`	1.646 ± 0.017	1.626	1.673	3.42 ± 0.04
`cairo-native (standalone AOT)`	0.652 ± 0.001	0.651	0.654	1.36 ± 0.01
`cairo-native (standalone AOT with -march=native)`	0.481 ± 0.002	0.480	0.485	1.00

Benchmark for program `fib_2M`

Open benchmarks

Command	Mean [s]	Min [s]	Max [s]	Relative
`Cairo-vm (Rust, Cairo 1)`	10.876 ± 0.080	10.810	11.070	1336.70 ± 18.12
`cairo-native (embedded AOT)`	1.178 ± 0.023	1.141	1.215	144.75 ± 3.24
`cairo-native (embedded JIT using LLVM's ORC Engine)`	1.126 ± 0.006	1.116	1.137	138.40 ± 1.76
`cairo-native (standalone AOT)`	0.008 ± 0.000	0.008	0.009	1.03 ± 0.02
`cairo-native (standalone AOT with -march=native)`	0.008 ± 0.000	0.008	0.008	1.00

Benchmark for program `logistic_map`

Open benchmarks

Command	Mean [s]	Min [s]	Max [s]	Relative
`Cairo-vm (Rust, Cairo 1)`	1.908 ± 0.009	1.892	1.924	28.83 ± 0.16
`cairo-native (embedded AOT)`	1.254 ± 0.012	1.239	1.275	18.95 ± 0.18
`cairo-native (embedded JIT using LLVM's ORC Engine)`	1.436 ± 0.016	1.419	1.475	21.70 ± 0.24
`cairo-native (standalone AOT)`	0.107 ± 0.000	0.107	0.107	1.62 ± 0.00
`cairo-native (standalone AOT with -march=native)`	0.066 ± 0.000	0.066	0.067	1.00

codecov-commenter · 2024-05-20T21:00:44Z

Codecov Report

Attention: Patch coverage is 90.32258% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 92.36%. Comparing base (cd75446) to head (9d9b7a8).

Files	Patch %	Lines
src/executor.rs	50.00%	2 Missing ⚠️
src/bin/cairo-native-run.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #610      +/-   ##
==========================================
+ Coverage   92.33%   92.36%   +0.02%     
==========================================
  Files         109      109              
  Lines       34097    34090       -7     
==========================================
+ Hits        31485    31486       +1     
+ Misses       2612     2604       -8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

azteca1998 · 2024-05-22T11:14:10Z

src/executor.rs

+            Some(return_ptr) => JitValue::from_jit(return_ptr, type_id, registry),
+            None => {
+                #[cfg(target_arch = "x86_64")]
+                let value = JitValue::from_jit(return_ptr.unwrap(), type_id, registry);


I just realized that this unwrap here would always fail if it wasn't unreachable.
It would always fail because the match's branch means that it's always None, and it's unreachable because x86 only returns values in two registers at most (128 bits, not 248).

Maybe we should change it to unreachable!()? (here and in the felt)

Good catch! @azteca1998
I added the unreachable!(), but I think I would be good to make a refactor in another PR that changes the fn parse_result so it return a Result, what do you think?

Returning a result in the case of reaching an unreachable part of the code? I've never seen the point of doing that.
However, if it's a refactor that avoids the need to have an unreachable!() statement due to a change in the design (and not pushing it into a result) then I'm all for it.

Yes, I meant the refactor that avoids unreachable!()
I will take it next!

This reverts commit 7d375b5.

igaray · 2024-05-23T12:07:25Z

src/executor.rs

@@ -647,15 +647,24 @@ fn parse_result(
            Some(return_ptr) => JitValue::from_jit(return_ptr, type_id, registry),
            None => {
                #[cfg(target_arch = "x86_64")]
-                let value = JitValue::from_jit(return_ptr.unwrap(), type_id, registry);
+                unreachable!();


Can you please add a comment explaining the difference behaviour across architectures?

Something like this should work:

Suggested change

unreachable!();

// Since x86_64's return values hold at most two different 64bit registers,

// everything bigger than u128 will be returned by memory, therefore making

// this branch unreachable on that architecture.

unreachable!();

Great!
Done 9d9b7a8

Pedro Fontana added 3 commits May 17, 2024 17:36

Implement Bytes31 is_complex

a6a291c

implement JitValue::Bytes31 and parse result

c548dcd

Add test

d9a7c9c

pefontana linked an issue May 17, 2024 that may be closed by this pull request

Implement CoreTypeConcrete::Bytes31 return type #596

Closed

Merge branch 'main' into add-bytes31-return-type

0eb857e

Fix arch x86

f7d67f5

pefontana marked this pull request as ready for review May 20, 2024 19:28

pefontana requested review from edg-l, igaray, azteca1998, jrchatruc, entropidelic, fmoletta and Oppen as code owners May 20, 2024 19:28

Fix upcast test

28a83bc

edg-l previously approved these changes May 21, 2024

View reviewed changes

edg-l added the review-ready A PR that is ready for review label May 21, 2024

azteca1998 previously approved these changes May 22, 2024

View reviewed changes

Merge branch 'main' into add-bytes31-return-type

c05f7ec

pefontana dismissed stale reviews from azteca1998 and edg-l via c05f7ec May 22, 2024 14:23

Pedro Fontana and others added 5 commits May 22, 2024 12:13

Change parse_result

7d375b5

Revert "Change parse_result"

c32697a

This reverts commit 7d375b5.

Change parse_result

49bced1

unreachable

a556d0a

Merge branch 'main' into add-bytes31-return-type

4462888

pefontana requested review from azteca1998 and edg-l May 22, 2024 20:29

igaray requested changes May 23, 2024

View reviewed changes

pefontana and others added 2 commits May 23, 2024 19:51

Merge branch 'main' into add-bytes31-return-type

4460ada

doc unreachable

9d9b7a8

pefontana requested a review from igaray May 23, 2024 22:55

azteca1998 approved these changes May 28, 2024

View reviewed changes

juanbono approved these changes May 28, 2024

View reviewed changes

juanbono enabled auto-merge May 28, 2024 20:54

igaray approved these changes May 29, 2024

View reviewed changes

juanbono added this pull request to the merge queue May 29, 2024

Merged via the queue into main with commit 89d44a1 May 29, 2024
9 checks passed

juanbono deleted the add-bytes31-return-type branch May 29, 2024 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Bytes31 return type #610

Add Bytes31 return type #610

pefontana commented May 17, 2024 •

edited

Loading

github-actions bot commented May 17, 2024 •

edited

Loading

codecov-commenter commented May 20, 2024 •

edited

Loading

azteca1998 May 22, 2024

pefontana May 22, 2024

azteca1998 May 23, 2024

pefontana May 23, 2024

igaray May 23, 2024

azteca1998 May 23, 2024

pefontana May 23, 2024

-                unreachable!();
+                // Since x86_64's return values hold at most two different 64bit registers,
+                // everything bigger than u128 will be returned by memory, therefore making
+                // this branch unreachable on that architecture.
+                unreachable!();

Add Bytes31 return type #610

Add Bytes31 return type #610

Conversation

pefontana commented May 17, 2024 • edited Loading

Checklist

github-actions bot commented May 17, 2024 • edited Loading

Benchmarking results

Benchmark for program factorial_2M

Benchmark for program fib_2M

Benchmark for program logistic_map

codecov-commenter commented May 20, 2024 • edited Loading

Codecov Report

azteca1998 May 22, 2024

Choose a reason for hiding this comment

pefontana May 22, 2024

Choose a reason for hiding this comment

azteca1998 May 23, 2024

Choose a reason for hiding this comment

pefontana May 23, 2024

Choose a reason for hiding this comment

igaray May 23, 2024

Choose a reason for hiding this comment

azteca1998 May 23, 2024

Choose a reason for hiding this comment

pefontana May 23, 2024

Choose a reason for hiding this comment

pefontana commented May 17, 2024 •

edited

Loading

github-actions bot commented May 17, 2024 •

edited

Loading

Benchmark for program `factorial_2M`

Benchmark for program `fib_2M`

Benchmark for program `logistic_map`

codecov-commenter commented May 20, 2024 •

edited

Loading