Uniquify module names that are common to Top & Model #1442

joonho3020 · 2023-04-14T23:48:40Z

This PR addresses #1388. After running split-module-files.py, it runs a additional script uniqify-module-names.py to uniquify the modules that are common to Model & Top. It updates the model_module_hierarchy.json accordingly.

Related PRs / Issues:

Type of change:

Bug fix
New feature
Other enhancement

Impact:

RTL change
Software change (RISC-V software)
Build system change
Other

Contributor Checklist:

Did you set main as the base branch?
Is this PR's title suitable for inclusion in the changelog and have you added a changelog:<topic> label?
Did you state the type-of-change/impact?
Did you delete any extraneous prints/debugging code?
Did you mark the PR with a changelog: label?
(If applicable) Did you update the conda .conda-lock.yml file if you updated the conda requirements file?
(If applicable) Did you add documentation for the feature?
(If applicable) Did you add a test demonstrating the PR?

(If applicable) Did you mark the PR as Please Backport?

harrisonliew

This script nominally looks correct. However, I believe that you should roll this into the already existing split-module-files.py script. These 3 lines are already processing the intersection, so might as well perform the uniquification in there?

joonho3020 · 2023-04-17T17:14:34Z

This script nominally looks correct. However, I believe that you should roll this into the already existing split-module-files.py script. These 3 lines are already processing the intersection, so might as well perform the uniquification in there?

I agree that that would also work, but I put this as a separate script because we would like to remove this script eventually when CIRCT supports top & harness separation. @abejgonzalez did you file a CIRCT issue regarding this? If not, I'll make a issue.

scripts/uniqify-module-names.py

abejgonzalez · 2023-04-19T18:11:22Z

scripts/uniqify-module-names.py

+  return fnames
+
+def update_filelist(cur_file, new_file):
+  sh.sed("-i", f"s/\b{cur_file}\b/{new_file}/", os.path.join(args.gcpath, args.mod_filelist))


Is there any issues with this and paths with /? If so, maybe we should use a different delimiter (;?) for sed

Ditto for other sed's

can you explain what you mean by issues with paths with /?

The default delimiter of sed is /, so if you are replacing things that have a / within them the sed will break. For example, if you want to replace /hi/this/is/abe with /hi/this/is/joonho the following sed command would break: sed -i s//hi/this/is/abe//hi/this/is/joonho/g .... So instead you could do something like sed -i 's#/hi/this/is/abe#/hi/this/is/joonho#g' ... (with # as the new delimiter)

Oh, but the cur_file & new_file here are just files (not paths) so I assume this is okay?

scripts/uniqify-module-names.py

harrisonliew · 2023-04-20T04:29:17Z

Also found by @allpan3: in sim.mk, can you make this change (comm no longer needed):

-       for x in $$(comm -23 <(cat $(MODEL_MODS_FILELIST) $(MODEL_BB_MODS_FILELIST) | sort -u) <(sort $(VLSI_RTL))) $(MODEL_SMEMS_FILE) $(SIM_FILE_REQS); do \
+       for x in $$(cat $(MODEL_MODS_FILELIST) $(MODEL_BB_MODS_FILELIST) | sort -u) $(MODEL_SMEMS_FILE) $(SIM_FILE_REQS); do \

abejgonzalez

SFTM. Pending @harrisonliew (+co) checking

scripts/uniqify-module-names.py

harrisonliew

@allpan3 can you give this PR one more try?

conda-reqs/conda-lock-reqs/conda-requirements-riscv-tools-linux-64.conda-lock.yml

This reverts commit 611de45.

This reverts commit 632a895.

generators/testchipip

This reverts commit bb6f8be.

allpan3 · 2023-04-20T23:49:39Z

@allpan3 can you give this PR one more try?

Tested on commit 41592fd.
make buildfile was able to complete. Running synthesis now and will update later.

allpan3 · 2023-04-21T00:18:15Z

@allpan3 can you give this PR one more try?

Tested on commit 41592fd. make buildfile was able to complete. Running synthesis now and will update later.

Encountering this issue in synthesis:

Error   : Could not resolve reference. [CDFG-431] [elaborate]
        : The unresolved reference is for instance 'ram_ext' in file '/bwrcq/C/allpan/chipyard/vlsi/generated-src/chipyard.TestHarness.HPURocketConfig/gen-collateral/Queue_16.sv' on line 136.
        : Use 'set_db hdl_error_on_blackbox false' (in CUI mode) or 'set_attribute hdl_error_on_blackbox false /' (in legacy mode) to cause a warning, rather than an error, when a blackbox is found.
Info    : Unable to elaborate the design. [ELAB-4]

Looks like ram_combMem_12_TestHarness_UNIQUIFIED is defined in ram_combMem_12_TestHarness_UNIQUIFIED.sv file, which is not part of the file list fed to Genus.

Queue_16.sv:  ram_combMem_12_TestHarness_UNIQUIFIED ram_ext (	// @[Decoupled.scala:273:95]
ram_combMem_12_TestHarness_UNIQUIFIED.sv:module ram_combMem_12_TestHarness_UNIQUIFIED(	// @[Decoupled.scala:273:95]

None of the *_UNIQUIFIED.sv files are in the file list. Is this the issue?

allpan3 · 2023-04-21T01:54:50Z

Was 57d7e55 supposed to address this issue?
Still hitting the same error.

Looks like all input files are listed in /bwrcq/C/allpan/chipyard/vlsi/build/chipyard.TestHarness.HPURocketConfig-ChipTop/inputs.yml, but the *_UNIQUIFIED.sv files aren't.

./example-vlsi -e /bwrcq/C/allpan/chipyard/vlsi/env-bwrc.yml -p /bwrcq/C/allpan/chipyard/vlsi/example-tools.yml -p /bwrcq/C/allpan/chipyard/vlsi/tstech28.yml -p /bwrcq/C/allpan/chipyard/vlsi/build/chipyard.TestHarness.HPURocketConfig-ChipTop/inputs.yml -p /bwrcq/C/allpan/chipyard/vlsi/build/chipyard.TestHarness.HPURocketConfig-ChipTop/sram_generator-output.json -p /bwrcq/C/allpan/chipyard/vlsi/tstech28_sram_macros/sram_generator-output.yml --obj_dir /bwrcq/C/allpan/chipyard/vlsi/build/chipyard.TestHarness.HPURocketConfig-ChipTop syn

joonho3020 · 2023-04-21T02:30:33Z

Can you tell me what file is fed into genus?

allpan3 · 2023-04-21T03:20:23Z

Can you tell me what file is fed into genus?

This is what hammer takes
/bwrcq/C/allpan/chipyard/vlsi/build/chipyard.TestHarness.HPURocketConfig-ChipTop/inputs.yml
It contains the list of files used for simulation and synthesis.

And here is the log file /bwrcq/C/allpan/chipyard/vlsi/syn.log. You can search for read_hdl.

allpan3 · 2023-04-21T03:33:46Z

Can you tell me what file is fed into genus?

I'm guessing something related to these lines needs to be changed?
syn.f needs to contain the uniquified files.

joonho3020 · 2023-04-21T05:36:47Z

I think the uniquified files are not suppose to be fed into synthesis tools. The problem is that Queue_16.sv should use ram_combMem_12 instead of ram_combMem_12_TestHarness_UNIQUIFIED, but the suffix was added for some reason. Can you come up with a config that replicates this issue? @allpan3

allpan3 · 2023-04-21T06:18:23Z

I think the default RocketConfig will hit this issue. The project I was running was just an accelerator on top of the rocket chip. @joey0320

joonho3020 · 2023-04-21T06:32:43Z

I think the default RocketConfig will hit this issue. The project I was running was just an accelerator on top of the rocket chip. @joey0320

I checked with RocketConfig & don't see cases where the above error happens. Can you post your {top, model}_module_hierarchy.json?

allpan3 · 2023-04-21T06:42:27Z

hmm yea you are right. I tried RocketConfig and it's good.
Somehow I'm hitting this issue when I'm running my project.
Here are the files.

/bwrcq/C/allpan/chipyard/vlsi/generated-src/chipyard.TestHarness.HPURocketConfig/model_module_hierarchy.json
/bwrcq/C/allpan/chipyard/vlsi/generated-src/chipyard.TestHarness.HPURocketConfig/top_module_hierarchy.json

And I see these lines printed:

/bwrcq/C/allpan/chipyard/scripts/uniqify-module-names.py \
	--top-filelist /bwrcq/C/allpan/chipyard/vlsi/generated-src/chipyard.TestHarness.HPURocketConfig/chipyard.TestHarness.HPURocketConfig.top.f \
	--mod-filelist /bwrcq/C/allpan/chipyard/vlsi/generated-src/chipyard.TestHarness.HPURocketConfig/chipyard.TestHarness.HPURocketConfig.model.f \
	--gen-collateral-path /bwrcq/C/allpan/chipyard/vlsi/generated-src/chipyard.TestHarness.HPURocketConfig/gen-collateral \
	--model-hier-json /bwrcq/C/allpan/chipyard/vlsi/generated-src/chipyard.TestHarness.HPURocketConfig/model_module_hierarchy.json \
	--out-model-hier-json /bwrcq/C/allpan/chipyard/vlsi/generated-src/chipyard.TestHarness.HPURocketConfig/model_module_hierarchy.uniquified.json \
	--dut ChipTop \
	--model TestHarness
......
sed -i s/"module ram_combMem_12"/"module ram_combMem_12_TestHarness_UNIQUIFIED"/ /bwrcq/C/allpan/chipyard/vlsi/generated-src/chipyard.TestHarness.HPURocketConfig/gen-collateral/ram_combMem_12_TestHarness_UNIQUIFIED.sv
echo "/bwrcq/C/allpan/chipyard/vlsi/generated-src/chipyard.TestHarness.HPURocketConfig/gen-collateral/ram_combMem_12_TestHarness_UNIQUIFIED.sv" >> /bwrcq/C/allpan/chipyard/vlsi/generated-src/chipyard.TestHarness.HPURocketConfig/chipyard.TestHarness.HPURocketConfig.model.f
sed -i s/"ram_combMem_12"/"ram_combMem_12_TestHarness_UNIQUIFIED"/ /bwrcq/C/allpan/chipyard/vlsi/generated-src/chipyard.TestHarness.HPURocketConfig/gen-collateral/Queue_16.sv

joonho3020 · 2023-04-21T06:50:25Z

Oh I meant the json files themselves lol

allpan3 · 2023-04-21T06:54:43Z

Oh I meant the json files themselves lol

np. There you go.
model_module_hierarchy.json.txt
top_module_hierarchy.json.txt

joonho3020 · 2023-04-21T16:42:30Z

Oh I meant the json files themselves lol

np. There you go. model_module_hierarchy.json.txt top_module_hierarchy.json.txt

Thanks, I pushed a fixed version, can you try with this one?

abejgonzalez · 2023-04-21T21:38:58Z

PR https://github.com/ucb-bar/chipyard/pull/1448/files addresses the CI failures. So can ignore the current CI failures in this PR

allpan3 · 2023-04-21T21:52:01Z

Oh I meant the json files themselves lol

np. There you go. model_module_hierarchy.json.txt top_module_hierarchy.json.txt

Thanks, I pushed a fixed version, can you try with this one?

Synthesis looks good now.

joonho3020 added 3 commits April 14, 2023 11:32

add python script, wip

240c551

script to unqify modules

e59ed50

rm print

c204e4a

joonho3020 changed the title ~~Uniquify names~~ Uniquify module names that are common to Top & Model Apr 14, 2023

joonho3020 requested review from abejgonzalez and harrisonliew April 14, 2023 23:49

joonho3020 added 2 commits April 14, 2023 21:24

add sh to conda env

632a895

update conda-lock file again

611de45

harrisonliew requested changes Apr 17, 2023

View reviewed changes

joonho3020 added 2 commits April 17, 2023 10:06

bump testchipip

bb6f8be

rm wrong assertion

04f4260

abejgonzalez requested changes Apr 19, 2023

View reviewed changes

jerryz123 added the changelog:fixed label Apr 19, 2023

harrisonliew requested changes Apr 20, 2023

View reviewed changes

scripts/uniqify-module-names.py Outdated Show resolved Hide resolved

abejgonzalez mentioned this pull request Apr 20, 2023

Final Release Prep #1448

Merged

16 tasks

fixes

9ce0467

abejgonzalez approved these changes Apr 20, 2023

View reviewed changes

harrisonliew reviewed Apr 20, 2023

View reviewed changes

scripts/uniqify-module-names.py Outdated Show resolved Hide resolved

harrisonliew reviewed Apr 20, 2023

View reviewed changes

scripts/uniqify-module-names.py Outdated Show resolved Hide resolved

harrisonliew reviewed Apr 20, 2023

View reviewed changes

conda-reqs/conda-lock-reqs/conda-requirements-riscv-tools-linux-64.conda-lock.yml Outdated Show resolved Hide resolved

joonho3020 added 3 commits April 20, 2023 16:16

address comments

41592fd

Revert "update conda-lock file again"

10e6413

This reverts commit 611de45.

Revert "add sh to conda env"

776419e

This reverts commit 632a895.

harrisonliew reviewed Apr 20, 2023

View reviewed changes

generators/testchipip Outdated Show resolved Hide resolved

Revert "bump testchipip"

dfe3487

This reverts commit bb6f8be.

joonho3020 force-pushed the uniquify-names branch from 90c0b1f to dfe3487 Compare April 20, 2023 23:42

harrisonliew approved these changes Apr 20, 2023

View reviewed changes

oops

57d7e55

fix

4d988a2

joonho3020 merged commit 15e22d9 into main Apr 21, 2023

joonho3020 deleted the uniquify-names branch April 21, 2023 22:47

joonho3020 restored the uniquify-names branch April 21, 2023 23:23

tymcauley mentioned this pull request Nov 5, 2023

[FIRRTL] Dut and Harness separation flag for firtool llvm/circt#5066

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uniquify module names that are common to Top & Model #1442

Uniquify module names that are common to Top & Model #1442

joonho3020 commented Apr 14, 2023

harrisonliew left a comment

joonho3020 commented Apr 17, 2023

abejgonzalez Apr 19, 2023

abejgonzalez Apr 19, 2023

joonho3020 Apr 20, 2023

abejgonzalez Apr 20, 2023

joonho3020 Apr 20, 2023

harrisonliew commented Apr 20, 2023 •

edited

Loading

abejgonzalez left a comment

harrisonliew left a comment

allpan3 commented Apr 20, 2023

allpan3 commented Apr 21, 2023 •

edited

Loading

allpan3 commented Apr 21, 2023

joonho3020 commented Apr 21, 2023

allpan3 commented Apr 21, 2023

allpan3 commented Apr 21, 2023

joonho3020 commented Apr 21, 2023

allpan3 commented Apr 21, 2023 •

edited

Loading

joonho3020 commented Apr 21, 2023

allpan3 commented Apr 21, 2023

joonho3020 commented Apr 21, 2023

allpan3 commented Apr 21, 2023

joonho3020 commented Apr 21, 2023

abejgonzalez commented Apr 21, 2023

allpan3 commented Apr 21, 2023

Uniquify module names that are common to Top & Model #1442

Uniquify module names that are common to Top & Model #1442

Conversation

joonho3020 commented Apr 14, 2023

harrisonliew left a comment

Choose a reason for hiding this comment

joonho3020 commented Apr 17, 2023

abejgonzalez Apr 19, 2023

Choose a reason for hiding this comment

abejgonzalez Apr 19, 2023

Choose a reason for hiding this comment

joonho3020 Apr 20, 2023

Choose a reason for hiding this comment

abejgonzalez Apr 20, 2023

Choose a reason for hiding this comment

joonho3020 Apr 20, 2023

Choose a reason for hiding this comment

harrisonliew commented Apr 20, 2023 • edited Loading

abejgonzalez left a comment

Choose a reason for hiding this comment

harrisonliew left a comment

Choose a reason for hiding this comment

allpan3 commented Apr 20, 2023

allpan3 commented Apr 21, 2023 • edited Loading

allpan3 commented Apr 21, 2023

joonho3020 commented Apr 21, 2023

allpan3 commented Apr 21, 2023

allpan3 commented Apr 21, 2023

joonho3020 commented Apr 21, 2023

allpan3 commented Apr 21, 2023 • edited Loading

joonho3020 commented Apr 21, 2023

allpan3 commented Apr 21, 2023

joonho3020 commented Apr 21, 2023

allpan3 commented Apr 21, 2023

joonho3020 commented Apr 21, 2023

abejgonzalez commented Apr 21, 2023

allpan3 commented Apr 21, 2023

harrisonliew commented Apr 20, 2023 •

edited

Loading

allpan3 commented Apr 21, 2023 •

edited

Loading

allpan3 commented Apr 21, 2023 •

edited

Loading