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

[VAN-798] correct missing "run" functions and subcmp arguments #71

Merged
merged 6 commits into from
Nov 8, 2023

Conversation

tim-hoffman
Copy link

also stabilize lit test output ordering for flattened function version

@tim-hoffman tim-hoffman self-assigned this Nov 6, 2023
@tim-hoffman tim-hoffman marked this pull request as ready for review November 7, 2023 03:23
circuit_passes/src/passes/loop_unroll/body_extractor.rs Outdated Show resolved Hide resolved
//CHECK-NEXT: %call.fr_sub = call i256 @fr_sub(i256 %4, i256 1)
//CHECK-NEXT: %5 = getelementptr i256, i256* %subc_[[X3]], i32 0
//CHECK-NEXT: store i256 %call.fr_sub, i256* %5, align 4
//CHECK-NEXT: call void @Sum_0_run([0 x i256]* %sub_[[X3]])
Copy link

Choose a reason for hiding this comment

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

Ah I see, interesting consequence of us immediately emitting the run call after assigning all the input signals is that we effectively have 4 copies of the same loop body when we could have 2. Avoiding this in all cases seems impossible, but at least in this case, we would avoid it if we generated the run call right before using the subcomponent's output rather than right after the last input signal is assigned (since the assignment is outside of the loop). My guess is that this is not a very important optimization, though (and it's certainly outside the scope of this PR---just some thought I had while trying to understand why there were more functions than I expected).

Copy link
Author

Choose a reason for hiding this comment

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

We would have to be careful to only call the run function once even if the output is read more than once or there is more than one output signal.

Copy link
Collaborator

Choose a reason for hiding this comment

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

By construction we know that all input signals will be assigned before we can read any outputs so I like Ian's idea in that regards but there are components that sometimes you don't need to read the output (i.e Num2Bits because its constrained within) so we will be missing the run function of that one in some cases

Base automatically changed from llvm-v2 to llvm November 8, 2023 15:39
@tim-hoffman tim-hoffman requested a review from iangneal November 8, 2023 16:01
Copy link

@iangneal iangneal left a comment

Choose a reason for hiding this comment

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

LGTM

@tim-hoffman tim-hoffman merged commit bb9aee1 into llvm Nov 8, 2023
3 checks passed
@tim-hoffman tim-hoffman deleted the th/van-798 branch November 8, 2023 17:55
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