steveicarus / iverilog

Icarus Verilog
https://steveicarus.github.io/iverilog/
GNU General Public License v2.0
2.85k stars 530 forks source link

Icarus freezes when analyzing program #1171

Open joaovam opened 1 month ago

joaovam commented 1 month ago

Icarus freezes when analyzing the program below:

module module_0 #(
    parameter id_1 = 32'd36
);
  initial begin

    if (id_1) begin
      if (id_1[(id_1[1'b0-{id_1, id_1} : id_1]!==id_1) : id_1]) id_1 <= 1;
    end
  end
endmodule

The output is the following:

warning: verinum::as_long() truncated 64 bits to 63, returns 9223371882235953116
warning: verinum::as_long() truncated 64 bits to 63, returns 9223371882235953116
caryr commented 1 month ago

This is invalid Verilog and the "could not find variable id_1 error" is valid though not as informative as it could be. You are performing a non-blocking assignment to the parameter id_1 and it's likely that when the compiler is searching for the variable it is only looking for a variable and is missing that there is a parameter already by that name. To be more helpful it should be reporting that you cannot perform a non-blocking assignment to a parameter.

Next is the hang. This appears to be happening because of the following code: (id_1[1'b0-{id_1, id_1} : id_1]!==id_1) This is creating a 64-bit value from {id_1, id_1}. This is then negated 1'b0-{id_1, id_1} and used as the MSB of a part select. It looks like this is being truncated to generate a very large positive value which is then creating a huge vector that you are then checking is not equal to id_1. To perform this comparison id_1 must also be extended to match the width of the huge vector and I am assuming the hang is really the compiler trying to create and compare these two monstrous values. I believe it will likely eventually finish or run out of memory. I'm running iverilog with the -delaborate flag right now and the output that is being dumped to a file is already over 100M in size and still growing.

Also, there is no reason part select code like you are showing should be used. Whatever is generating this code is broken. Have you tried running this code on a different simulator to see what other tools report?

caryr commented 1 month ago

FYI I killed the previous run since it was likely IO limited writing the huge debug value and just ran it normally, it eventually failed with a bad_alloc message so it ran out of memory as I was guessing it would. At best we should look into not generating billion bit vectors and reporting assignments to parameters are invalid.

I do not believe this code is demonstrating any bugs in iverilog. I will mark it as an enhancement so we can generate better messages.

joaovam commented 1 month ago

I was able to simplify the code based on your comment, I have updated it in the issue. I have tried this updated code in yosys and Jasper from Cadence and both were able to analyze it and elaborate it.

caryr commented 1 month ago

This is still invalid code and given that the other tools accept it with no issues implies they are not actually processing this as the standard dictates. Have you run this on other commercial simulators to see if they can process the code? It did fail on the one I tried.

The first issue is you still have a non-blocking assignment to a parameter which is not valid and the second is you are trying to create a value that is billions of times larger than what a system with 32GB could represent. Maybe the other tools are able to determine the part select is 100% out of range and are creating a lazy evaluation that can then be used to verify that the part select term is !== to the parameter id_1 resulting in true without actually expanding both variables to their expected total width as mandated in the standard.

Without extensive work to support lazy evaluation the most straight forward solution is to check for creating a value that is too large and then generating an error.

Here is simple Verilog code that shows what you are requesting:

module top;
  parameter id_1 = 32'd36;
  initial $display("[%0d:%0d]", 1'b0-{id_1, id_1}, id_1);
endmodule

This produces the output [18446743919090728924:36] which is 18.4 exa bits. Icarus is diligently trying to create this value and that is why it appears to hang until it eventually runs out of memory (both physical and virtual). If you let it run long enough it will run out of memory. Whatever generator you are using to create this code should not be creating values that cannot be represented factoring in the limitations of modern computer systems.

caryr commented 1 month ago

I've looked at this a little more and have confirmed Icarus is working as expected when you do not overflow the memory. On a machine that has defined long as 32bits and then adding the -fshort-enums flag to gcc to minimize the size of the enumeration we use to represent constant numbers I was able to get something that did not run out of memory.

There are still two remaining issues in the modified code you provided:

  1. You are still performing a non-blocking assignment to a parameter id_1 <= 1
  2. A part select is required to access bit(s) in MSB to LSB order. When the expression is evaluated the !== returns 1 for this case so the constant part select is id_1[1:36] which violates this requirement in the standard.

    The first expression shall address a more significant bit than the second expression.

What I think needs to be done from the Icarus side is switch the verinum enumeration to a class enum that only uses a single byte per bit instead of using an old style enumeration which defaults to an int (typically 4 bytes). The class enum was added in C++ 11 which is what we are currently using as the minimum requirement to compile Icarus. We should see if there are other places we use enumerations to see if they would benefit from class enums.

Next we should have a way to detect when the verinum to a long has been truncated and when this happens provide a more appropriate error message. We should also check that the bounds and size of an element can actually be represented in memory. The standard sets a suggested limit at 2^16 for the packed dimensions and 2^24 for the unpacked dimensions. I will look at what other simulators do before hard coding a limit.

Whatever generator you are using is generating garbage code and needs to be constrained better.