cucapra/dahlia

Possible regression: Incorrect Calyx IR generated for seq_mem_d1

Closed this issue · 6 comments

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

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

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

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?

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?

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

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