Skip to content

flow-control: Support lowering of EqualsLiteral node.#8131

Merged
liorgold2 merged 1 commit intomainfrom
pr/liorgold2/lior/flow-control/08269923
Aug 10, 2025
Merged

flow-control: Support lowering of EqualsLiteral node.#8131
liorgold2 merged 1 commit intomainfrom
pr/liorgold2/lior/flow-control/08269923

Conversation

@liorgold2
Copy link
Collaborator

@liorgold2 liorgold2 commented Aug 9, 2025

@reviewable-StarkWare
Copy link

This change is Reviewable

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-lowering/src/lower/mod.rs line 950 at r1 (raw file):

/// Lowers a semantic expression that is a literal, possibly including a negation.
fn lower_expr_literal_helper<'db>(

Suggestion:

/// Lowers a semantic expression that is a literal, possibly including a negation.
fn lower_expr_literal<'db>(
    ctx: &mut LoweringContext<'db, '_>,
    expr: &semantic::ExprLiteral<'db>,
    builder: &mut BlockBuilder<'db>,
) -> LoweringResult<'db, LoweredExpr<'db>> {
    log::trace!("Lowering a literal: {:?}", expr.debug(&ctx.expr_formatter));
    Ok(LoweredExpr::AtVariable(lower_expr_literal_helper(
        ctx,
        expr.stable_ptr.untyped(),
        expr.ty,
        &expr.value,
        builder,
    )))
}


fn lower_expr_literal_to_var_usage<'db>(

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-lowering/src/lower/flow_control/graph.rs line 118 at r1 (raw file):

    /// The literal to check against.
    pub literal: usize,
    /// A stable pointer to the first instance of the literal in the patterns.

What is the literal?

Suggestion:

a literal

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-lowering/src/lower/flow_control/lower_graph/lower_node.rs line 188 at r1 (raw file):

            ctx.ctx,
            literal_stable_ptr,
            felt252_ty,

Why can't the literal be u32?

Code quote:

 felt252_ty,

@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/08269923 branch from 53db34c to c8098e7 Compare August 10, 2025 10:49
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/eaebd1fa branch from 5c2ee32 to dd9f730 Compare August 10, 2025 10:49
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/08269923 branch from c8098e7 to 76e2a8d Compare August 10, 2025 11:37
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/eaebd1fa branch from dd9f730 to 0222f09 Compare August 10, 2025 11:37
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/08269923 branch from 76e2a8d to ee5c1b2 Compare August 10, 2025 12:00
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/eaebd1fa branch from 0222f09 to 1792553 Compare August 10, 2025 12:00
Copy link
Collaborator Author

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-lowering/src/lower/flow_control/graph.rs line 118 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

What is the literal?

It's the literal, as this struct is about comparing to a specific literal (this is the pub literal: usize member).
BTW (same for "the input value")


crates/cairo-lang-lowering/src/lower/flow_control/lower_graph/lower_node.rs line 188 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

Why can't the literal be u32?

For now, I've copied the current implementation, which always converts to felt252 before the comparisons. (Also, I'm only supporting felt252, since I'm not actually doing the conversion).

There's the following TODO:

// TODO(TomerStarkware): Use the same type of literal as the input, without the cast to
//   felt252.

which we should consider doing (I don't know what are the implications of doing it)


crates/cairo-lang-lowering/src/lower/mod.rs line 950 at r1 (raw file):

/// Lowers a semantic expression that is a literal, possibly including a negation.
fn lower_expr_literal_helper<'db>(

Done.

@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/08269923 branch from ee5c1b2 to cfbc537 Compare August 10, 2025 12:05
Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 5 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @liorgold2)

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @liorgold2)

@liorgold2 liorgold2 changed the base branch from pr/liorgold2/lior/flow-control/eaebd1fa to main August 10, 2025 13:22
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/08269923 branch from cfbc537 to 9c6d653 Compare August 10, 2025 13:22
@liorgold2 liorgold2 enabled auto-merge August 10, 2025 13:22
@liorgold2 liorgold2 added this pull request to the merge queue Aug 10, 2025
Merged via the queue into main with commit 4ba4bba Aug 10, 2025
96 checks passed
@orizi orizi deleted the pr/liorgold2/lior/flow-control/08269923 branch August 11, 2025 14:13
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.

3 participants