Cap table support #566

ErikFanderson · 2020-03-06T06:13:34Z

This allows for specifying a "cap table file" in the tech.json for older technologies that do not come with a qrc techfile. There should not be any issue with supplying both a cap table file and qrc tech file. The tool should always prioritize the qrc tech file. I also edited the function "get_and_prepend_path" inside the "process_library_filter" function so that if an empty path is retrieved from the tech.json then an empty string will be returned. This behavior appears to have already been expected in the "get_mmmc_qrc" function.

edwardcwang · 2020-03-06T08:35:07Z

Hi Erik, thank you for your contributions to Hammer! :)

A few things:

Just to clarify, are we referring to this format or another format for the cap table? https://github.com/The-OpenROAD-Project/PEX/blob/master/kits/nangate45/captables/NCSU_FreePDK_45nm.capTbl
Could you add a unit tests for this filter, following the format of the other unit tests for similar features?
It appears that the type checker is failing.

Thanks again for your contribution!

src/hammer-vlsi/hammer_vlsi/hammer_vlsi_impl.py

colinschmidt

Looks pretty reasonable.
I assume you have tested this locally?

colinschmidt · 2020-03-06T17:02:21Z

src/hammer-tech/hammer_tech.py

@@ -811,7 +811,7 @@ def process_library_filter(self,
        # Next, extract paths and prepend them to get the real paths.
        def get_and_prepend_path(lib: Library) -> Tuple[Library, List[str]]:
            paths = filt.paths_func(lib)
-            full_paths = list(map(lambda path: self.prepend_dir_path(path, lib), paths))
+            full_paths = list(map(lambda path: self.prepend_dir_path(path, lib) if path else '', paths))


Why did this change? Is this just a bugfix?

prepend_dir_path has an assertion that checks to make sure the path is not empty. I am not entirely sure why the assertion exists, but this was my solution to get an empty string to be returned

Hmm, how do empty paths end up showing up to begin with? And how is this cap table specific?

Well because you only need a cap_table_file OR a qrc_techfile then 1 of them can be empty. Previously hammer would error if you did not supply a qrc_techfile even though the function "generate_mmmc_script" already assumed that if there was no supplied qrc_techfile that an empty string would be retrieved when calling "get_mmmc_qrc". I guess this raises the question of whether we actually want an error to be thrown if neither of these files are supplied

Hm, seeing how other things use prepend_dir_path, I also don't see what the value of the len(path) > 0 assertion is. It is only used in certain cases (DRC/LVS decks, map files) but doesn't make whatever you specified in the tech json any more or less correct.

I think what we really should do is call an os.path.exists on the constructed path so that Python can catch bad paths in tech jsons before it's passed to the tool where sometimes at best only a warning is thrown.

If there are no qrc techfiles, then wouldn't get_mmmc_qrc() return an empty string since the read_libs call would return an empty list?

As a side note, we really need to detach the QRC techfile from the libraries (#315).

Maybe we can incorporate that larger change into this PR, so that we aren't creating more work we need to undo/fix later?

src/hammer-vlsi/hammer_vlsi/hammer_vlsi_impl.py

ErikFanderson · 2020-03-06T17:38:06Z

Hi Erik, thank you for your contributions to Hammer! :)

A few things:

1. Just to clarify, are we referring to this format or another format for the cap table? https://github.com/The-OpenROAD-Project/PEX/blob/master/kits/nangate45/captables/NCSU_FreePDK_45nm.capTbl

2. Could you add a unit tests for this filter, following the format of the other unit tests for similar features?

3. It appears that the type checker is failing.

Thanks again for your contribution!

Yes, that is the format I am referring to.
Yes, I will definitely do that. Please be patient with me as I sort through the testing infrastructure.
Yup, I will try to sort that out asap.

edwardcwang · 2020-03-06T21:46:01Z

Great! Might be good to add a link to https://github.com/The-OpenROAD-Project/PEX/blob/master/kits/nangate45/captables/NCSU_FreePDK_45nm.capTbl in the yaml comments/documentation for reference.

…e checking cap table

ErikFanderson · 2020-03-11T17:40:01Z

Update on this: Added a test for the filter in tech_test.py and also added a property to the Library stub class for type checking purposes. Not entirely sure why only some properties from the jsonschema generated Library class need to be defined in the stub class, but mypy no longer errors.

edwardcwang

Generally looks good but have a further question about this line.

edwardcwang · 2020-03-11T18:59:37Z

src/hammer-tech/hammer_tech.py

@@ -811,7 +811,7 @@ def process_library_filter(self,
        # Next, extract paths and prepend them to get the real paths.
        def get_and_prepend_path(lib: Library) -> Tuple[Library, List[str]]:
            paths = filt.paths_func(lib)
-            full_paths = list(map(lambda path: self.prepend_dir_path(path, lib), paths))
+            full_paths = list(map(lambda path: self.prepend_dir_path(path, lib) if path else '', paths))


Hmm, how do empty paths end up showing up to begin with? And how is this cap table specific?

edwardcwang · 2020-03-11T20:02:28Z

Can we also add https://github.com/The-OpenROAD-Project/PEX/blob/master/kits/nangate45/captables/NCSU_FreePDK_45nm.capTbl to a pydoc or something to keep track of the format?

initial cap table support added

b271095

harrisonliew reviewed Mar 6, 2020

View reviewed changes

src/hammer-vlsi/hammer_vlsi/hammer_vlsi_impl.py Show resolved Hide resolved

colinschmidt reviewed Mar 6, 2020

View reviewed changes

ErikFanderson added 2 commits March 10, 2020 22:09

removed space and reinserted indents

117d547

Added cap table test and added property in Library stub class for typ…

5ac4327

…e checking cap table

harrisonliew approved these changes Mar 11, 2020

View reviewed changes

edwardcwang requested changes Mar 11, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cap table support #566

Cap table support #566

ErikFanderson commented Mar 6, 2020

edwardcwang commented Mar 6, 2020

colinschmidt left a comment

colinschmidt Mar 6, 2020

ErikFanderson Mar 6, 2020 •

edited

Loading

edwardcwang Mar 11, 2020

ErikFanderson Mar 11, 2020 •

edited

Loading

harrisonliew Mar 11, 2020 •

edited

Loading

edwardcwang Mar 11, 2020

jwright6323 Mar 11, 2020

jwright6323 Mar 11, 2020

ErikFanderson commented Mar 6, 2020

edwardcwang commented Mar 6, 2020

ErikFanderson commented Mar 11, 2020

edwardcwang left a comment

edwardcwang Mar 11, 2020

edwardcwang commented Mar 11, 2020

Cap table support #566

Are you sure you want to change the base?

Cap table support #566

Conversation

ErikFanderson commented Mar 6, 2020

edwardcwang commented Mar 6, 2020

colinschmidt left a comment

Choose a reason for hiding this comment

colinschmidt Mar 6, 2020

Choose a reason for hiding this comment

ErikFanderson Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

edwardcwang Mar 11, 2020

Choose a reason for hiding this comment

ErikFanderson Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

harrisonliew Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

edwardcwang Mar 11, 2020

Choose a reason for hiding this comment

jwright6323 Mar 11, 2020

Choose a reason for hiding this comment

jwright6323 Mar 11, 2020

Choose a reason for hiding this comment

ErikFanderson commented Mar 6, 2020

edwardcwang commented Mar 6, 2020

ErikFanderson commented Mar 11, 2020

edwardcwang left a comment

Choose a reason for hiding this comment

edwardcwang Mar 11, 2020

Choose a reason for hiding this comment

edwardcwang commented Mar 11, 2020

ErikFanderson Mar 6, 2020 •

edited

Loading

ErikFanderson Mar 11, 2020 •

edited

Loading

harrisonliew Mar 11, 2020 •

edited

Loading