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

[Cider] Continuous assignments don't "trigger" when an if immediately follows an invoke #1314

Open
sampsyo opened this issue Dec 18, 2022 · 2 comments
Labels
C: Interpreter / Cider Calyx Interpreter & Debugger Type: Bug Bug in the implementation

Comments

@sampsyo
Copy link
Contributor

sampsyo commented Dec 18, 2022

First, an aside: Cider has been UNBELIEVABLY helpful when working on Advent of Code. I don't know how I'd manage without it—maybe by manually hacking $display calls into the generated Verilog?? Naw!

Anyway, I found a bug in the interpreter when working on cucapra/aoc2022-calyx#5.

Reduced Test Case

Here's a reduced test case, which unfortunately is not very short:

import "primitives/core.futil";
component main() -> () {
  cells {
    @external res = std_mem_d1(32, 1, 1);

    c = std_const(32, 4);
    w = std_wire(1);
    r = std_reg(32);

    nop_reg = std_reg(32);
  }

  wires {
    group set_res {
      res.write_en = 1'd1;
      res.write_data = 32'd50;
      set_res[done] = res.done;
    }

    // group nop {
    //   nop_reg.write_en = 1'd1;
    //   nop_reg.in = 32'd0;
    //   nop[done] = nop_reg.done;
    // }

    // Notably, packing up these assignments into a `comb group` and attaching
    // them to the `if` with `with` makes the problem go away.
    w.in = r.out > 32'd2 ? 1'd1;
    w.in = r.out <= 32'd2 ? 1'd0;
  }

  control {
    seq {
      // Notably, replacing this `invoke` with the equivalent explicit group
      // makes the problem go away.
      invoke r(in=c.out)();

      // Notably, adding even a "no-op" group here makes the problem go away.
      // nop;

      if w.out { set_res; }
    }
  }
}

Here is a minimal input data file:

{
    "res": {
        "format": {
            "is_signed": false,
            "numeric_type": "bitnum",
            "width": 32
        },
        "data": [0]
    }
}

Here's a command to reproduce the problem:

$ fud e -vv red.futil --to interpreter-out -s verilog.data red.data.json

Note that the res memory contains 0. You can also run the same code using Verilator:

$ fud e -vv red.futil --to dat -s verilog.data red.data.json

This gives the correct value for the output memory, 50.

Load-Bearing Code

The bug seems to require all of these things to be present:

  • The invoke of the register to write it. (Manually "compiling" the invoke away into a plain group that writes to the register makes the problem go away.)
  • The if. (I haven't been able to reproduce any problems when just using the signal directly, as opposed to in an if condition.)
  • The if must immediately follow the invoke in the control program. (Adding a no-op group activation in there makes the problem go away.)
  • Continuous assignments. (Those two assignments to w.in appear to be necessarily continuous. If I instead pack them up into a comb group and then do if w.out with my_great_comb_group, the problem goes away.)

So this is a very specific situation. That also means it certainly can't be very important, so @EclecticGriffin should really feel free to de-prioritize it. 😄

@sampsyo sampsyo added Priority: Low Don't do these during a deadline Type: Bug Bug in the implementation C: Interpreter / Cider Calyx Interpreter & Debugger labels Dec 18, 2022
@rachitnigam
Copy link
Contributor

Cool find! @sampsyo if its okay with you, can we remove the "low priority" label? I think the kind of code above is more likely to be generated from frontends like @andrewb1999's pipeline generator which needs to more carefully control the scheduling of if and while requiring omission of the with comb statement that causes code like this to not occur in Dahlia or other frontends

@sampsyo
Copy link
Contributor Author

sampsyo commented Dec 19, 2022

Sure!

@sampsyo sampsyo removed the Priority: Low Don't do these during a deadline label Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Interpreter / Cider Calyx Interpreter & Debugger Type: Bug Bug in the implementation
Projects
None yet
Development

No branches or pull requests

2 participants