Conversation
90a6ce9 to
ad76396
Compare
|
@cburgdorf Or, we could add support for multiple returns to the sonatina ir, and add arithmetic instructions that output two values. Minor nitpick, the fe intrinsics could be generic: |
crates/codegen/src/sonatina/lower.rs
Outdated
|
|
||
| /// Returns the masking info for a sub-256-bit intrinsic, or `None` for 256-bit | ||
| /// types that don't need masking. | ||
| fn sub_word_mask(callee_name: &str) -> Option<SubWordMask> { |
There was a problem hiding this comment.
fe should create sonatina ir that uses sonatina int sizes (i8, i16, etc) that match the fe int size, instead of always using i256, and let sonatina handle any masking etc. I don't think it currently does this, so doing it in fe would be fine for now.
There was a problem hiding this comment.
I don't think it currently does this
Yes, right. The normal arithmetic doesn't do it yet but I now did so for the checked arithmetic. I'll add another commit to expand it to the non-checked arithmetic.
I'm leaning toward allowing multiple returns in sonatina ir. This would be useful for optimizing functions that return an aggregate; we could transform them into functions that return multiple scalar values. I'll start working on this today. |
ad76396 to
1eb230d
Compare
|
@codex review |
I refactored that but used the existing |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1eb230d6e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1eb230d to
4bac28c
Compare
|
@cburgdorf fe-lang/sonatina#210 let overflow_block = builder.append_block();
let ok_block = builder.append_block();
let [sum, overflow] = builder.insert_uaddo(lhs, rhs);
builder.insert_inst_no_result(Br::new(is, overflow, overflow_block, ok_block));new ops are: EVM-specific ops: (div-by-zero = 0) |
|
I also have this branch that adds a legalization pass for the evm backend, to properly handle smaller int sizes (with appropriate masks, sign extension, etc): fe-lang/sonatina#209 So, the sonatina codegen in fe can just emit ops on small int sizes. I'll try to get both of these checked and merged today. |
|
@sbillig I can look deeper into the Sonatina tomorrow evening or on Monday. For now I'm just cleaning up the current state to address all the remaining issues that codex highlighted. |
|
@codex review |
|
@sbillig I updated this to your latest Sonatina changes. It does depend on this extra fix though fe-lang/sonatina#213 |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 817a45a4ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0409f74d7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Summary
+,-,*, unary-) now perform overflow/underflow checks at runtime for all integer types, reverting the transaction on overflow — matching Solidity 0.8+ behavior__checked_{add,sub,mul,neg}_<type>extern intrinsics incore::numthat the MIR lowers operator expressions into, with overflow detection handled directly in the Sonatina and Yul backendsWrappingAdd/WrappingSub/WrappingMulandSaturatingAdd/SaturatingSub/SaturatingMultraits incore::opsas opt-in escape hatches for intentional wrapping/saturating semanticssdiv), signed modulo (smod), and arithmetic right shift (sar) support in the Sonatina backendchecked_arithmetic_methods.fe,checked_arithmetic_regressions.fe,signed_arithmetic.fewith both happy-path andshould_reverttestsArchitecture: layered approach across backends
The implementation uses a layered strategy that keeps overflow semantics consistent while respecting the different needs of the Yul and Sonatina backends:
Core stdlib (
core::num): Operator trait impls (e.g.Add for u8) now call__checked_add_u8instead of__add_u8. These__checked_*functions are declared asexternintrinsics — they have no Febody. The unchecked
__add_*variants remain available and are used by theWrapping*/Saturating*trait impls.MIR: When the MIR lowers a call to a
__checked_*extern, it attaches aCheckedIntrinsicmetadata struct (carrying theCheckedArithmeticOpand the concreteTyId) to theCallOrigin. This givesbackends typed information about what overflow check is needed without embedding any particular lowering strategy at the MIR level.
Sonatina backend: The
lower_checked_intrinsicfunction pattern-matches on theCheckedIntrinsicto emit the arithmetic instruction followed by an inline overflow test (e.g.result < lhsfor unsignedadd, sign-flag analysis for signed add) and a conditional branch to a revert block. All overflow logic is emitted as native Sonatina IR — no function calls, no runtime helpers.
Yul backend: The YUL backend takes a fundamentally different approach. The overflow logic is implemented in Fe itself via
core::num_yul— a set of generic helper functions (
checked_add_unsigned_impl,checked_sub_signed_impl, etc.) that use theIntWordtrait to operate on the word-level representation of any integer type. These generichelpers are monomorphized per type via thin wrappers (e.g.
checked_add_u8,checked_neg_i256). When the Yul emitter encounters a__checked_*extern call, it resolves it to the correspondingcore::num_yul::checked_*function, which gets compiled to Yul like any other Fe function. This means the Yul backend gets overflow checking "for free" through normal Fe compilation, without needing anyspecial intrinsic handling.
@sbillig That part ☝️, I'm most unsure about. I remember from the last time we spoke that you set we should keep what we have implement in Fe but keep it YUL specific so this is why now the YUL backend uses
num_yul.fefor all the safemath stuff whereas Sonatina doesn't go that extra route.