Skip to content

Conversation

@liorgold2
Copy link
Collaborator

@liorgold2 liorgold2 commented Jul 31, 2025

@reviewable-StarkWare
Copy link

This change is Reviewable

@liorgold2 liorgold2 marked this pull request as draft July 31, 2025 16:11
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/827e842e branch from d85abdd to 2167b5b Compare July 31, 2025 16:41
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/97600111 branch from e52fd15 to fdcebc2 Compare July 31, 2025 16:41
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/827e842e branch from 2167b5b to 66b9e67 Compare July 31, 2025 16:45
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/97600111 branch 2 times, most recently from ad6a371 to f6296a6 Compare August 1, 2025 09:41
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/827e842e branch from 66b9e67 to 50a9b00 Compare August 1, 2025 09:41
@liorgold2 liorgold2 marked this pull request as ready for review August 3, 2025 06:27
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-lowering/src/borrow_check/test_data/borrow_check line 853 at r1 (raw file):

  (v12: test::A) <- struct_construct(v9{`a`})
End:
  Return(v12, v11)

are there cases where this would cause a regression?

or would following optimizations remove the issue?
(or sierra-gen end of function "push" would make this irrelevant?)

Code quote:

  Goto(blk3, {v6 -> v9})

blk2:
Statements:
  (v10: core::integer::u32) <- struct_destructure(v0{`if true { bar(@a.x); }`})
End:
  Goto(blk3, {v10 -> v9})

blk3:
Statements:
  (v11: ()) <- struct_construct()
  (v12: test::A) <- struct_construct(v9{`a`})
End:
  Return(v12, v11)

@liorgold2 liorgold2 changed the base branch from pr/liorgold2/lior/flow-control/97600111 to main August 3, 2025 07:53
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/827e842e branch from 50a9b00 to 9216e99 Compare August 3, 2025 07:53
@liorgold2 liorgold2 changed the base branch from main to pr/liorgold2/lior/flow-control/d1a5121c August 3, 2025 07:53
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: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-lowering/src/borrow_check/test_data/borrow_check line 853 at r1 (raw file):

Previously, orizi wrote…

are there cases where this would cause a regression?

or would following optimizations remove the issue?
(or sierra-gen end of function "push" would make this irrelevant?)

I think @ilyalesokhin-starkware said this is the preferred way. It's hard for me to say if it'll cause regression in some cases. I think the new way decreases the size of the remapping (better to remap the necessary members of a struct than the entire struct)

@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/827e842e branch from 9216e99 to f8f7057 Compare August 3, 2025 08:15
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/d1a5121c branch from 23cb152 to ceb7386 Compare August 3, 2025 08:15
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/827e842e branch from f8f7057 to 18d2b05 Compare August 3, 2025 09:18
@liorgold2 liorgold2 changed the base branch from pr/liorgold2/lior/flow-control/d1a5121c to main August 3, 2025 09:18
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

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 added this pull request to the merge queue Aug 3, 2025
Merged via the queue into main with commit a67bdec Aug 3, 2025
49 checks passed
@orizi orizi deleted the pr/liorgold2/lior/flow-control/827e842e branch August 4, 2025 11:48
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.

5 participants