llvm / circt

Circuit IR Compilers and Tools
https://circt.org
Other
1.67k stars 297 forks source link

[SV] Miss-complication by reordering side-effect ops #3028

Open uenoku opened 2 years ago

uenoku commented 2 years ago
hw.module @Foo(%clock:i1, %a:i8, %b:i8) {
  %reg = sv.reg : !hw.inout<i8> 
  %fd = hw.constant 0x80000002 : i32
  sv.always posedge %clock {
    sv.bpassign %reg, %a : i8
    %x = sv.read_inout %reg : !hw.inout<i8>
    sv.bpassign %reg, %b : i8
    sv.fwrite %fd, "%d"(%x) : i8
  }
}

Current output:

module Foo(
  input       clock,
  input [7:0] a,
              b);

  reg [7:0] reg_0;

  always @(posedge clock) begin
    reg_0 = a;
    reg_0 = b;
    $fwrite(32'h80000002, "%d", reg_0);
  end // always @(posedge)
endmodule

This is incorrect because the value of a should be printed. I guess we have to create a temporary register if we can't use automatic.

uenoku commented 2 years ago

With disallowLocalVariable option, Prepare swaps bpassign and read_inout which is obviously incorrect. So we have to fix this bug at Prepare.

; IR after Prepare
  sv.always posedge %clock {
    sv.bpassign %reg, %a : i8
    sv.bpassign %reg, %b : i8
    %0 = sv.read_inout %reg : !hw.inout<i8>
    sv.fwrite %c-2147483646_i32, "%d"(%0) : i8
  }

The general solution would be, as suggested by @darthscsi, to introduce a temporary register at Prepare so that we don't have to modify ExportVerilog a lot.

mikeurbach commented 2 years ago

Is this introduced in https://github.com/llvm/circt/commit/300e24414e95a53bbf10ab10f50b836b7e31324a? That's the most recent change to read_inout ordering. I may have approved that PR to hastily.

+1 for introducing constructs in Prepare to Export can be simpler.

uenoku commented 2 years ago

No, https://github.com/llvm/circt/commit/300e24414e95a53bbf10ab10f50b836b7e31324a is not source of this bug. I confirmed reordering happens before the commit as well. However, it seems https://github.com/llvm/circt/commit/300e24414e95a53bbf10ab10f50b836b7e31324a moves read_inout's position so this might be doing something incorrect. I think we have to check wether there is no blocking assignment in the same block.

uenoku commented 2 years ago

I tried this on firtool sifive/0/0/1~sifive/0/6/0. It has never been compiled correctly:pensive: