Updates to remove the generate statements #54
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aehart
reviewed
Oct 10, 2023
aehart
reviewed
Nov 7, 2023
WriteVHDLSyntax.py
Outdated
|
|
||
| for memMod in memList: | ||
|
|
||
| mem = memMod.list |
There was a problem hiding this comment.
@aryd The MemModule class doesn't have a member called list. Did you mean inst?
Suggested change
| mem = memMod.list | |
| mem = memMod.inst |
|
Andrew,
I think this must be memMod.inst. I would need to logon and check, but I think this is what I did in other instances. I guess this code was not executed? Otherwise I should have seen an error?
-Anders
Anders Ryd
***@***.******@***.***>
On Nov 7, 2023, at 12:19 PM, Andrew Hart ***@***.******@***.***>> wrote:
@aehart commented on this pull request.
________________________________
In WriteVHDLSyntax.py<#54 (comment)>:
- string_mem += " NUM_PAGES".ljust(str_len)+"=> " + str(2**bxbitwidth) + "\n"
- string_mem += " )\n"
- string_mem += " port map (\n"
- string_mem += " CLK".ljust(str_len)+"=> CLK,\n"
- string_mem += " ADDR".ljust(str_len)+"=> "+mtypeB+"_mem_AV_readaddr(var),\n"
- string_mem += " DATA".ljust(str_len)+"=> "+mtypeB+"_mem_AV_dout(var),\n"
- string_mem += " READ_EN".ljust(str_len)+"=> "+mtypeB+"_mem_A_enb(var),\n"
- string_mem += " NENT_ARR".ljust(str_len)+"=> "+mtypeB+"_mem_AA" + ("A" if is_binned else "") + "V_dout_nent(var),\n"
- string_mem += " DONE".ljust(str_len)+"=> "+proc+"_DONE\n"
- string_mem += " );\n"
- string_mem += " end generate "+mtypeB+"_loop;\n\n"
+ memList = memDict[mtypeB]
+
+ for memMod in memList:
+
+ mem = memMod.list
@aryd<https://github.com/aryd> The MemModule class doesn't have a member called list. Did you mean inst?
⬇️ Suggested change
- mem = memMod.list
+ mem = memMod.inst
—
Reply to this email directly, view it on GitHub<#54 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABI4LTI7BRICP4LL5H35C5TYDIKNNAVCNFSM6AAAAAA5N3VFKOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMJXGQ2DMNJSHE>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
@aryd I only noticed this because I updated the submodule in cms-L1TK/firmware-hls#298 so that we could see if the CI tests were successful. They failed as soon as emData/download.sh was executed… So this code is definitely executed, at least by the calls to generator_hdl.py in that script. |
|
I’ll have to check - I thought I had tested this…I will take a look later today.
-Anders
Anders Ryd
***@***.******@***.***>
On Nov 7, 2023, at 1:53 PM, Andrew Hart ***@***.******@***.***>> wrote:
I think this must be memMod.inst. I would need to logon and check, but I think this is what I did in other instances. I guess this code was not executed? Otherwise I should have seen an error?
@aryd<https://github.com/aryd> I only noticed this because I updated the submodule in cms-L1TK/firmware-hls#298<cms-L1TK/firmware-hls#298> so that we could see if the CI tests were successful. They failed as soon as emData/download.sh was executed…
So this code is definitely executed, at least by the calls to generator_hdl.py in that script.
—
Reply to this email directly, view it on GitHub<#54 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABI4LTN56EH3S6CETEMGSEDYDIVNDAVCNFSM6AAAAAA5N3VFKOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJYGQ2DIOBUGI>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Andrew,
I looked into this now. I concluded that .list should be changed to .inst. Now the download.sh script runs.
I pushed the fix to the branch RemoveGenerate in project_generation_script. As the PR has already been closed I presume I should open a new PR?
I also pushed the new project_generatio_script version to the branch RemoveGenerate in the main HLS repo.
(I’m not 100% sure why I missed this this, but I find that working with the submodules and the download.sh script is very confusing as every time you run the download.sh script it reinitializes the submodule and changes the branch you have checked out in the project_generation_script. I spent 1/2 hour this morning being confused about this as the code in my project_generation_scripts are changed every time I read download.sh….)
-Anders
Anders Ryd
***@***.******@***.***>
On Nov 7, 2023, at 1:53 PM, Andrew Hart ***@***.******@***.***>> wrote:
I think this must be memMod.inst. I would need to logon and check, but I think this is what I did in other instances. I guess this code was not executed? Otherwise I should have seen an error?
@aryd<https://github.com/aryd> I only noticed this because I updated the submodule in cms-L1TK/firmware-hls#298<cms-L1TK/firmware-hls#298> so that we could see if the CI tests were successful. They failed as soon as emData/download.sh was executed…
So this code is definitely executed, at least by the calls to generator_hdl.py in that script.
—
Reply to this email directly, view it on GitHub<#54 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABI4LTN56EH3S6CETEMGSEDYDIVNDAVCNFSM6AAAAAA5N3VFKOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJYGQ2DIOBUGI>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
This PR is still open, so I will take a look at it and approve if everything is OK now.
Thanks. The CI jobs failed with some errors that look transient. I just restarted them, so that we can see if they're happy with the changes to SectorProcessor. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR modifies the code such that the memory modules are not instantiated using "generate" statements, but rather each instance is written out in the SectorProcessor.vhd file. This was already done for the processing modules so this was not as large a change as I had originally thought. This change will then allow to simplify how we implement the memory modules where we will be able to have different number of memory copies for the combined memory modules.