byuccl / spydrnet

A flexible framework for analyzing and transforming FPGA netlists. Official repository.
https://byuccl.github.io/spydrnet
BSD 3-Clause "New" or "Revised" License
86 stars 20 forks source link

Incorrect handling of big-endian declaration (is_downto=false) #221

Closed ganeshgore closed 3 months ago

ganeshgore commented 4 months ago

In the current master branch, the following netlist with big-endian bundle declaration is incorrectly handled.
My understanding is then big-endian bundle is defined is_downto=False and pins will be placed in reverse order in the python list. For example, in the following example. the python list for cable upto.wires[0] = <verilog_net_of_upto[1]> upto.wires[0] = <verilog_net_of_upto[0]>

Input netlist

module sample(upto);
    input [0:1] upto;
    module1 instance2(.pin1(upto[1]), .pin2(upto[0]));
endmodule

Output netlist

module sample(upto);
    input [1:0] upto;
    module1 instance2(.pin1(upto[1]), .pin2(upto[0]));
endmodule
ganeshgore commented 4 months ago

This test seems to be wrong according to is_downto assumption https://github.com/byuccl/spydrnet/blob/d5b9bd8313dce511fdc583c3451d290ba0ac3178/tests/spydrnet/parsers/verilog/tests/test_verilogParser.py#L602-L603

ganeshgore commented 4 months ago

I think we can do this, there are two approaches https://github.com/byuccl/spydrnet/blob/d5b9bd8313dce511fdc583c3451d290ba0ac3178/tests/spydrnet/parsers/verilog/tests/test_verilogParser.py#L737

  1. Maintaining the consistency between the Verilog index and Python list index

    • I believe this is how the current parser is expected to run, because when you define [1:0]sample_c or [0:1]sample_c, in both scenarios the python list sample_c.wire[0] points to verilog net sample_c[0] and sample_c.wire[1] points to verilog net sample_c[1]
    • This means during manipulation if the is_downto property Is changed we need to reverse the _wires list, it can be achieved by extending is_downto property definition in bundle.py
  2. Consider is_donwto argument only during parsing and composing

    • This will make verilog index and python list index different, but we can add the property "verilog_index" to get the actual Verilog index of the wire or pin
    • In this the [1:0]sample_c declaration will be parsed as sample_c.wire[0]->sample_c[0] and sample_c.wire[1]->sample_c[1] but
    • [0:1]sample_cdeclaration will be parsed assample_c.wire[0]->sample_c[1]andsample_c.wire[1]->sample_c[0]`
    • During composing is_downto flag will be used to get the correct index
    • In this scenario, you don't need to worry about is_downto flag changing during the manipulation

I can fix this I would like to know which one is the preferred implementation

jacobdbrown4 commented 4 months ago

Thanks for submitting this. Which approach did you take in your pull request #222 ?

ganeshgore commented 4 months ago

I went with the first option that seems less intrusive.

ganeshgore commented 3 months ago

I am still checking different cases, if you want please close this I will create new issue if I find any new bug