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

ZIR-304: support Zhlt::BackOp in picus mode #139

Merged
merged 7 commits into from
Jan 14, 2025
Merged

Conversation

mars-risc0
Copy link
Contributor

No description provided.

@github-actions github-actions bot changed the title support Zhlt::BackOp in picus mode ZIR-304: support Zhlt::BackOp in picus mode Dec 19, 2024
Copy link
Contributor

@jacobdweightman jacobdweightman left a comment

Choose a reason for hiding this comment

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

Awesome start!

size_t distance = back.getDistance().getZExtValue();
AnySignal signal = signalize(freshName(), back.getType());
if (distance > 0) {
declareSignals(signal, /*isInput=*/true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I told you the wrong thing here. Backs shouldn't really be "inputs" because they aren't part of the call interface — doing that would mean we need to have corresponding arguments when we generate calls to the module. I asked Shankara to add a new predicate called assume-deterministic that marks a signal as deterministic without making it part of the call interface. This probably calls for a change to declareSignals. Here's a test case Shankara and I discussed:

Input:

component Foo(reg: NondetReg) {
    Reg(r@1)
}

Expected output (probably):

(prime-number 2013265921)

(begin-module Foo)
(input reg_layout_super)
(input reg_super)
(output layout_super_super)
(output result_super_super_super)
(output result_super_reg_super)
(assume-deterministic reg_back1)
(call [layout_super_super result_super_super_super result_super_reg_super] Reg [reg_back1])
(end-module)


// CHECK: (prime-number 2013265921)
// CHECK: (begin-module Count)
// CHECK: (input x0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the full output for this test, x0 corresponds to the argument first: Val rather than the back. I think we should make a filecheck assertion about all of the inputs and signals corresponding to the back (x1_*).

zirgen/compiler/picus/picus.cpp Show resolved Hide resolved
@mars-risc0 mars-risc0 force-pushed the mars/add-picus-backops branch 4 times, most recently from 3e1c083 to 33dc293 Compare January 13, 2025 19:55
Copy link
Contributor

@jacobdweightman jacobdweightman left a comment

Choose a reason for hiding this comment

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

What's in here is sound and reasonable, though it looks like the case of zero distance backs still isn't handled. I'd be okay to merge what's in here once CI passes, though.

@mars-risc0 mars-risc0 force-pushed the mars/add-picus-backops branch from a2b0a69 to cb1eeae Compare January 14, 2025 20:08
@mars-risc0 mars-risc0 marked this pull request as ready for review January 14, 2025 21:18
@mars-risc0 mars-risc0 merged commit 52f3fac into main Jan 14, 2025
10 checks passed
@mars-risc0 mars-risc0 deleted the mars/add-picus-backops branch January 14, 2025 21:23
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.

2 participants