RTimothyEdwards / magic

Magic VLSI Layout Tool
Other
469 stars 99 forks source link

Erroneous messages for duplicate cells. #227

Open d-m-bailey opened 1 year ago

d-m-bailey commented 1 year ago

During caravel make ship, cells are read from multiple locations.

        load user_project_wrapper; \
        property LEFview true; \
        property GDS_FILE $(UPRJ_ROOT)/gds/user_project_wrapper.gds; \
        property GDS_START 0; \
        load $(UPRJ_ROOT)/mag/user_id_programming; \
        load $(UPRJ_ROOT)/mag/user_id_textblock; \
        load $(CARAVEL_ROOT)/macros/simple_por/maglef/simple_por; \
        load $(UPRJ_ROOT)/mag/caravel_core; \
        load $(CARAVEL_ROOT)/mag/caravel -dereference; \

The mag files contain an explicit reference for the first cell loaded, but implicit references for subsequent cells.

use sky130_fd_sc_hd__diode_2  ANTENNA__061__A0 $PDKPATH/libs.ref/sky130_fd_sc_hd/mag
timestamp 1665323087
transform -1 0 6716 0 1 2720
box -38 -48 222 592
use sky130_fd_sc_hd__diode_2  ANTENNA__062__B
timestamp 1665323087
transform -1 0 6072 0 1 1632
box -38 -48 222 592

It appears that these implicit references are not recorded properly and cause a false mismatch when subsequent cells are read which results in a cell rename.

Duplicate cell in housekeeping:  Instance of cell sky130_fd_sc_hd__nand2_1.mag is from path /home/user/mpw-9/dependencies/pdks/sky130A/libs.ref/sky130_fd_sc_hd/mag but cell was previously read from /home/user/mpw-9/caravel_user_project3/mag/dependencies/pdks/sky130A/libs.ref/sky130_fd_sc_hd/mag.
Cell name conflict:  Renaming original cell to sky130_fd_sc_hd__nand2_1#0.

The previously read location does not exist - I think, it has been recorded incorrectly. /home/user/mpw-9/dependencies/pdks/sky130A/libs.ref/sky130_fd_sc_hd/mag is the correct PDK location. The recorded previous location /home/user/mpw-9/caravel_user_project3/mag is the location of the block that was opened and has no sub-directory dependencies.

This error only occurs if there are implicit instances in the mag file. If there is only one instance of a cell in the previously loaded block, there do not appear to be any errors.

Running make ship with the caravel/Makefile substitutions at the top, should recreate the problem. (There are other problems with the Makefile though).

RTimothyEdwards commented 1 year ago

The "implicit" references, as you call them, cannot be the issue. When reading a .mag file, magic reads a cell only once, the first time that cell is encountered. For the rest of that file, the cell has already been read in, which is why the reference is not repeated---it would be redundant to provide the same reference each time, and it would be an error to provide the same cell with two different references in the same file (it would be an impossibility unless the file was hand-edited).

RTimothyEdwards commented 1 year ago

But I do agree that there is some issue where the cell has been read in from a file but somehow has a cd_file entry that is not a location of anything, and seems to be some scrambled concoction of $PDKPATH and other path components. It looks like that might be caused by the lines in database/DBio.c that have the comment "Reconstruct file from path and cellname"---that is the only place where the cd_file record does not reflect the actual file that was read. But it would have to be an earlier conflict that caused the original name change. Are there earlier error or warning messages?

d-m-bailey commented 1 year ago

So there may be a couple issues. The mag cells in the user project area contain incorrect relative paths.

use sky130_fd_sc_hd__diode_2  ANTENNA_1 dependencies/pdks/sky130A/libs.ref/sky130_fd_sc_hd/mag
timestamp 1676037725
transform 1 0 175904 0 1 11968
box -38 -48 222 592

Relative paths should be calculated with respect to the top mag file directory and not merely the matching portion. $PDKPATH references should probably be kept as is. However, when reading this entry, it appears that an error is only generated if the cell already existed. If this is a newly read cell, no error is flagged even though this is an invalid path. The data may have been found on one of the search paths, but the data read is still being recorded as if it was at the bad path. This yields the previously mentioned error:

Duplicate cell in housekeeping:  Instance of cell sky130_fd_sc_hd__nand2_1.mag is from path /home/user/mpw-9/dependencies/pdks/sky130A/libs.ref/sky130_fd_sc_hd/mag but cell was previously read from /home/user/mpw-9/caravel_user_project3/mag/dependencies/pdks/sky130A/libs.ref/sky130_fd_sc_hd/mag.
Cell name conflict:  Renaming original cell to sky130_fd_sc_hd__nand2_1#0.

The previously read cell does not exist at that location.

On the other hand, if the cell is read from the PDK first, we get a message that the new path does not exist.

Duplicate cell in user_proj_example:  Instance of cell sky130_fd_sc_hd__diode_2.mag is from path /home/kanobailey/mpw-9/caravel_user_project3/mag/dependencies/pdks/sky130A/libs.ref/sky130_fd_sc_hd/mag but cell was previously read from /home/kanobailey/mpw-9/dependencies/pdks/sky130A/libs.ref/sky130_fd_sc_hd/mag.
New path does not exist and will be ignored.

So we need to check:

  1. What messages, if any, are displayed when the cell does not exist at a relative path. Is a silent path search performed? (I've seen bad file path errors for missing absolute path references, but not for relative paths)
  2. When and how are cell references saved as relative locations in mag files.
  3. When are cell references saved as PDK references.
RTimothyEdwards commented 1 year ago

Okay, I figured out how to duplicate the issue with a simple local setup: (1) Copy a library cell (e.g., sky130_fd_sc_hd__and2_1) to the current working directory (2) Create a test layout containing the library cell and save it. (3) Change the path of the library cell use in the test layout to invalid/path. (4) From a fresh start of magic, load the test layout (with the erroneous path). This will load without issue because the subcell has not been expanded. (5) Load the local version of the subcell. (6) cellname filepath now shows the invalid path.

So: Step (5) should have detected that the cell existed and had a path that was not the path specified on the load command. If that cell had never actually been loaded (which can be determined from a flag setting), then it should load it from the new location, but what it should do that it's not doing is to replace the file path when it does.

d-m-bailey commented 1 year ago

Excellent!

RTimothyEdwards commented 1 year ago

Eh. . . Would be if I could repeat the steps and get the same result, which I can't.

d-m-bailey commented 1 year ago

@RTimothyEdwards Tested with 8.3.384 and still get a mag file with incorrect references.

use sky130_fd_sc_hd__diode_2  ANTENNA_1 dependencies/pdks/sky130A/libs.ref/sky130_fd_sc_hd/mag
timestamp 1676037725
transform 1 0 175904 0 1 11968
box -38 -48 222 592

The execution directory is somewhere under /home/user/mpw-9/caravel_user_project3 and the PDK_ROOT is /home/user/mpw-9/dependencies/pdks.

When writing the mag file, the current relative reference calculation appears to remove the matching hierarchies from the reference path, which is incorrect. If the hierarchy of the parent cell is totally contained in the subcell reference, then the parent hierarchy could be removed from the subcell reference, but partial matching doesn't make sense and prevents portability.

Maybe the PDKPATH should be checked before the relative path calculation, too.

RTimothyEdwards commented 1 year ago

I will check what's in the code; according to what I recall was the intended behavior, the code should have noticed that both parent and child exist under the $HOME directory at /home/user/mpw-9, but also should have counted the number of directories to get back up to the common point, so at worst it should have ended up with a valid path to something like ../dependencies/pdks/sky130A/libs.ref/sky130_fd_sc_hd/mag. I agree, though, that checking $PDKPATH first (before checking the home path) makes the most sense.