Why does VPR prevent clock driving both data and clock pins?

[mithro]

There is a check in the clustering code shown below; https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/dcc8468fe9a159307128b27589f19f471aee66f0/vpr/src/pack/cluster.cpp#L774-L796

It claims that a clock being used for a LUT input would break the cluster. This means that there is code in read_circuit which deliberately breaks circuits using the following; https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/dcc8468fe9a159307128b27589f19f471aee66f0/vpr/src/base/read_circuit.cpp#L125-L133

(Which was added in https://github.com/mithro/vtr-verilog-to-routing/commit/6ccf516d32f3f9c30dd438950b892fd9d858a76d).

We hit this issue using the following Verilog code;

module top (
    input  clk,
    output LED1,
    output LED2,
    output LED3,
    output LED4,
    output LED5
);

   localparam BITS = 4;
   localparam LOG2DELAY = 22;

   reg [BITS+LOG2DELAY-1:0] counter = 0;
   reg [BITS-1:0]       outcnt;

   always @(posedge clk) begin
      counter <= counter + 1;
      outcnt <= counter >> LOG2DELAY;
   end

   wire bout;
   memory m1 (counter[LOG2DELAY-1], bout);

   assign LED1 = bout;
   assign {LED2, LED3, LED4, LED5} = outcnt;
endmodule

module memory (
           input clk,
           output bout
           );

   localparam DEPTH = 5;
   localparam LEN = 1<<(DEPTH-1);

   wire [15:0]        data;

   reg [DEPTH-1:0]   cnt = 0;

  SB_RAM40_4K #(
            .INIT_0(256'h0000000100000001000000010001000000000000000100000000000100000001),
            .INIT_1(256'h0000000100000001000000010000000100000001000000010000000100000001),
            .INIT_2(256'hxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx),
            .INIT_3(256'hxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx),
            .INIT_4(256'hxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx),
            .INIT_5(256'hxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx),
            .INIT_6(256'hxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx),
            .INIT_7(256'hxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx),
            .INIT_8(256'hxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx),
            .INIT_9(256'hxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx),
            .INIT_A(256'hxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx),
            .INIT_B(256'hxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx),
            .INIT_C(256'hxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx),
            .INIT_D(256'hxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx),
            .INIT_E(256'hxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx),
            .INIT_F(256'hxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx),
            .READ_MODE(32'sd1),
            .WRITE_MODE(32'sd1)
          ) mem  (
              .RADDR({ 2'h0, cnt}),
              .RCLK(clk),
              .RCLKE(1'h1),
              .RDATA(data),
              .RE(1'h1),
              .WCLK(clk),
              .WCLKE(0)
                 );

   always @(posedge clk) begin
      cnt <= cnt +1;
   end

   assign bout = data[0];

endmodule // memory

Which while I agree it is questionable to drive a block pin using the output of the counter, should still kind of work.

So, I thought I would see what breaks by disabling this check. However, when I disabled this check, everything still seemed to work?

Will things only break is a LUT output is used to drive a clock within the same tile? The iCE40 actually has routing which makes it pretty flexible to drive clocks of circuits. Should that be the check instead?

[vaughnbetz]

This is a very old check from the original t-vpack and I don't believe it is relevant anymore. It was originally intended to prevent me mis-counting the number of inputs to a cluster (and hence it's legality) for a corner case where a clock fed both data and the clock pin. The legality code is much more general and sophisticated so I doubt this still applied; even if it does apply it might lead to an unroute in a rare corner case, so not catastrophic. Suggest Tim delete and run the regtests, and if all looks OK then commit it. Can leave a warning saying this looks odd, but the clusterer should continue and the latch should not be inserted.

[kmurray]

I've checked and everything in both the vtr_reg_basic and vtr_reg_strong regressions test pass with these checks disabled.

As a result I'm going to delete the relevant checks and 'fix-up' code as Vaughn suggested.

[mithro]

Thanks for fixing this! :tada: