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

Possible regression: Incorrect Calyx IR generated for seq_mem_d1 #422

Closed
Mark1626 opened this issue Mar 5, 2024 · 6 comments · Fixed by #401
Closed

Possible regression: Incorrect Calyx IR generated for seq_mem_d1 #422

Mark1626 opened this issue Mar 5, 2024 · 6 comments · Fixed by #401
Labels
Comp: Calyx Issue related to the Calyx backend

Comments

@Mark1626
Copy link
Collaborator

Mark1626 commented Mar 5, 2024

Calyx IR for seq_mem_d1 uses methods which were renamed in the current upstream Calyx

The following code

decl A: ubit<32>[8];
decl B: ubit<32>[8];
decl v: ubit<32>[1];

for (let i: ubit<4> = 0..8) {
  let dot = A[i] * B[i];
} combine {
  v[0] += dot;
}

generated Calyx IR (truncated)

import "primitives/core.futil";
import "primitives/memories/seq.futil";
import "primitives/binary_operators.futil";
component main() -> () {
  cells {
    @external(1) A = seq_mem_d1(32,8,4);
    A_read0_0 = std_reg(32);
    @external(1) B = seq_mem_d1(32,8,4);
    B_read0_0 = std_reg(32);
    add0 = std_add(32);
    add1 = std_add(4);
    bin_read0_0 = std_reg(32);
    const0 = std_const(4,0);
    const1 = std_const(4,7);
    const2 = std_const(1,0);
    const3 = std_const(1,0);
    const4 = std_const(4,1);
    dot_0 = std_reg(32);
    i0 = std_reg(4);
    le0 = std_le(4);
    mult_pipe0 = std_mult_pipe(32);
    red_read00 = std_reg(32);
    @external(1) v = seq_mem_d1(32,1,1);
  }
 ...
 ... // Truncated
    group let1<"promotable"=2> {
      A_read0_0.in = A.read_data;
      A_read0_0.write_en = A.read_done;
      let1[done] = A_read0_0.done;
      A.addr0 = i0.out;
      A.read_en = 1'd1;
    }
 ... // Truncated
 ...
 ...

seq_mem_d1 primitive is using read_done which is renamed to done. Similarly it's also using read_en and write_done

@rachitnigam
Copy link
Member

Ah yes, this is definitely wrong. It should be done and we should be generating content_en signals and writing write_en = 0 or write_en = 1 depending on whether we want to perform a writ

@Mark1626
Copy link
Collaborator Author

Mark1626 commented Mar 5, 2024

I'll take a stab at this. A question for Calyx maybe, I don't see seq_mem_d1 documented
https://docs.calyxir.org/libraries/core.html#memories

@Mark1626 Mark1626 added the Comp: Calyx Issue related to the Calyx backend label Mar 6, 2024
@Mark1626
Copy link
Collaborator Author

Mark1626 commented Mar 6, 2024

I created a branch with a fix but it seems like the existing PR #401 itself would solve this problem. Any reason why it hasn't been merged?

@rachitnigam rachitnigam linked a pull request Mar 6, 2024 that will close this issue
@rachitnigam
Copy link
Member

Ah, that should've been merged after the new release of Calyx. There is a syntax change needed on the branch before it works. Could you take a shot?

@Mark1626
Copy link
Collaborator Author

Mark1626 commented Mar 6, 2024

Syntax change in the generated IR? Sure I'll take a shot

@rachitnigam
Copy link
Member

Nope, the scala code needs a syntax change (because we're using Scala 3 now). I couldn't fix it from my phone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Comp: Calyx Issue related to the Calyx backend
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants