zachjs / sv2v

SystemVerilog to Verilog conversion
BSD 3-Clause "New" or "Revised" License
540 stars 52 forks source link

Yosys error with multiple wrapped casts #125

Closed nmoroze closed 3 years ago

nmoroze commented 3 years ago

Hi! If I run sv2v on the following SystemVerilog example:

module example  (
  input [1:0] in,
);

  typedef struct packed {
    logic [1:0]  a;
  } mystruct;

  mystruct out;

  assign out = '{
    a:  2'(in[1:0])
  };

endmodule

withsv2v example.sv > example.v followed by yosys -p 'read_verilog example.v' I get the following error: "ERROR: Non-constant function call in constant expression." However, I'd expect the Yosys front-end to parse the Verilog without any issues.

I noticed that the issue itself seems to be due to having multiple wrapped sv2v cast functions, in particular the following line in the output: assign out = sv2v_cast_2(sv2v_cast_2(in[1:0]));. Removing the second sv2v_cast_2 by hand makes Yosys parse the output fine.

A few additional observations I made while debugging:

I'm honestly not sure if this should be considered an issue in sv2v or Yosys (the extra function wrapping seems unnecessary, but not incorrect to me). However, since you wrote the commit that seems to introduce this problem into Yosys, I figured you might have some good insight either way!

Thanks in advance for taking a look!

zachjs commented 3 years ago

Thanks for the detailed report! This is indeed a regression I caused in Yosys (sorry!), but the appropriate fixes are already pending. See YosysHQ/Yosys#2562 which contains the larger discussion so far.

I am interested in taking a pass at the cast conversion to try to remove some of the unnecessary cast function calls. I believe there is at least one instance of a double function call of this style not due to the conversion in OpenTitan (not sure if it's in the Ibex core), so I suggest using my Yosys repeat-call branch for the time being.

I'd like to leave this issue open to track progress on the cast conversion.

nmoroze commented 3 years ago

Ah I see, I guess I should have checked the Yosys issues more closely first! Thanks as always for the prompt reply and for pointing me towards the fixes.

zachjs commented 3 years ago

The Yosys constant function evaluation issue should now be fixed on master. sv2v should also be producing substantially fewer unnecessary cast functions, though I believe there is still room for improvement. Please let me know what you find!

nmoroze commented 3 years ago

An unrelated Yosys issue (exact problem TBD) has been keeping me from using the latest Yosys with your patch, but it does look like the extra cast mentioned in this issue no longer appears. Everything works for me now even on the older version of Yosys!

zachjs commented 3 years ago

It sounds like this issue has been resolved. Thanks again!