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

Unexpected behavior with reading from seq_mem reference and invoke #2117

Open
polybeandip opened this issue Jun 6, 2024 · 3 comments
Open
Labels
C: Calyx Extension or change to the Calyx IL C: calyx-py Items that have to do with the builder library Type: Bug Bug in the implementation

Comments

@polybeandip
Copy link
Collaborator

polybeandip commented Jun 6, 2024

This issue is about divergent behavior between icarus-verilog, verilator, and the Calyx interpreter.

Here's a toy eDSL program and it's associated data file:

# bug.py
import calyx.builder as cb

def component(prog):
    comp = prog.component("comp")
    
    A = comp.seq_mem_d1("A", 32, 1, 1, is_ref=True)
    B = comp.seq_mem_d1("B", 32, 1, 1, is_ref=True)

    a = comp.reg(32) 
    zero = comp.const("zero", 1, 0) 

    read_A = comp.mem_load_d1(A, zero.out, a, "read") 
    write_B = comp.mem_store_d1(B, zero.out, a.out, "write") 

    comp.control += [read_A, write_B]

    return comp

def insert_main(prog):
    main = prog.component("main")
    
    A = main.seq_mem_d1("A", 32, 1, 1, is_external=True)
    B = main.seq_mem_d1("B", 32, 1, 1, is_external=True)

    comp = component(prog)
    comp = main.cell("comp", comp)

    main.control += [cb.invoke(comp, ref_A=A, ref_B=B)]

if __name__ == "__main__":
    prog = cb.Builder()
    insert_main(prog)
    prog.program.emit()
# bug.data
{
  "A": {
    "data": [5],
    "format": {
      "numeric_type": "bitnum",
      "is_signed": false,
      "width": 32
    }
  },
  "B": {
    "data": [0],
    "format": {
      "numeric_type": "bitnum",
      "is_signed": false,
      "width": 32
    }
  }
}

This program should read 5 from memory A and write this value into memory B. However, depending on whether it's run with icarus-verilog, verilator, or the calyx interpreter, we find different results:

Verilator:

am3327@havarti:/scratch/anshuman/calyx$ fud e bug-after-compile-invoke.futil   -s verilog.data calyx-py/test/correctness/bug.data   --to dat --through verilog -v -q
{
  "cycles": 5,
  "memories": {
    "A": [
      5
    ],
    "B": [
      5
    ]
  }
}

Icarus-verilog:

am3327@havarti:/scratch/anshuman/calyx$ fud e bug-after-compile-invoke.futil   -s verilog.data calyx-py/test/correctness/bug.data   --to dat --through icarus-verilog -v -q
{
  "cycles": 5,
  "memories": {
    "A": [
      5
    ],
    "B": [
      0
    ]
  }
}

Calyx interpreter:

am3327@havarti:/scratch/anshuman/calyx$ fud e bug-after-compile-invoke.futil   -s verilog.data calyx-py/test/correctness/bug.data   --to interpreter-out -v -q
{
  "main": {
    "A": [
      5
    ],
    "B": [
      5
    ]
  }
}

(run on Havarti by @anshumanmohan)

verilator and the Calyx interpreter produce the correct result, while icarus-verilog does not.

It appears this behavior is specific to reading from sequential memories passed as references to a component called via invoke. For example, running either of these programs with bug.data causes no issues.

A changed from seq to comb memory:

import calyx.builder as cb

def component(prog):
    comp = prog.component("comp")
    
    A = comp.comb_mem_d1("A", 32, 1, 1, is_ref=True)
    B = comp.seq_mem_d1("B", 32, 1, 1, is_ref=True)

    a = comp.reg(32) 
    zero = comp.const("zero", 1, 0) 

    read_A = comp.mem_load_d1(A, zero.out, a, "read") 
    write_B = comp.mem_store_d1(B, zero.out, a.out, "write") 

    comp.control += [read_A, write_B]

    return comp

def insert_main(prog):
    main = prog.component("main")
    
    A = main.comb_mem_d1("A", 32, 1, 1, is_external=True)
    B = main.seq_mem_d1("B", 32, 1, 1, is_external=True)

    comp = component(prog)
    comp = main.cell("comp", comp)

    main.control += [cb.invoke(comp, ref_A=A, ref_B=B)]

if __name__ == "__main__":
    prog = cb.Builder()
    insert_main(prog)
    prog.program.emit()

Memories no longer passed via reference through invoke:

import calyx.builder as cb

def component(prog):
    comp = prog.component("main")
    
    A = comp.seq_mem_d1("A", 32, 1, 1, is_external=True)
    B = comp.seq_mem_d1("B", 32, 1, 1, is_external=True)

    a = comp.reg(32) 
    zero = comp.const("zero", 1, 0) 

    read_A = comp.mem_load_d1(A, zero.out, a, "read") 
    write_B = comp.mem_store_d1(B, zero.out, a.out, "write") 

    comp.control += [read_A, write_B]

    return comp

if __name__ == "__main__":
    prog = cb.Builder()
    component(prog)
    prog.program.emit()

You can find the files referenced here on this branch:

@anshumanmohan
Copy link
Contributor

anshumanmohan commented Jun 6, 2024

Thanks @polybeandip, and congrats(? 😅) on finding an interesting issue! A few suggestions to make other readers' lives easier:

  • Consider linking directly to permalinks for the files you want people to look at.
  • I know you've also created the most obvious .futil file as well as the .futil file after running the compile-invoke pass. Consider pointing folks to permalinks for those, since those are the first two things people will want to see.
  • I know you have done a further piece of sleuthing that revealed that this all happens only when invoke is involved on a seq memory. Please document that, as I think that will really help the team!

@polybeandip polybeandip added C: Calyx Extension or change to the Calyx IL C: calyx-py Items that have to do with the builder library labels Jun 6, 2024
@anshumanmohan
Copy link
Contributor

Changes looking great, thanks for bearing with my somewhat pedantic request!

@anshumanmohan anshumanmohan added the Type: Bug Bug in the implementation label Jun 6, 2024
@sampsyo
Copy link
Contributor

sampsyo commented Jun 7, 2024

Hey, @polybeandip—REALLY nice work on the bug writeup here; this is a model of how to make a description like this clear and self-contained. 👏

This looks like a real doozy. It's especially delicious that Icarus and Verilator disagree, with is example number 54089454 of how Verilog is under-specified to such a degree that even very popular implementations routinely disagree. I feel like there is a strong chance that this problem exists in our Verilog backend rather than in the compiler lowering per se.

I don't know if it will reveal anything, but one potentially-useful next step here would be to do some test case reduction on the generated Calyx program, or perhaps the one produced after compile-invoke. Maybe we can produce the smallest possible (lowered) Calyx program that elicits this disagreement…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Calyx Extension or change to the Calyx IL C: calyx-py Items that have to do with the builder library Type: Bug Bug in the implementation
Projects
None yet
Development

No branches or pull requests

3 participants