search

llvm / circt

Circuit IR Compilers and Tools
https://circt.org
Other
1.62k stars 281 forks source link

[FIRRTL] in `replSeqMem` stands for "replace sequential memories" not "replicate sequential memories" #7384

Open youngar opened 1 month ago

youngar commented 1 month ago

We have some flags named shouldReplicateSequentialMemories, but AFAICT this is incorrectly named and should be called shouldReplaceSequentialMemories. See the search result: https://github.com/search?q=repo%3Allvm%2Fcirct%20shouldReplicateSequentialMemories&type=code

jpien13 commented 1 month ago

Hey, I was referred to this issue by colleagues of Megan Wach. I am a rising senior at college studying computer engineering. This would be my first issue within the CIRCT project. I'd be happy to try and tackle this one.

jpien13 commented 1 month ago

Hey @youngar I made the changes and it looks like I need permissions to push my branch:

remote: Permission to llvm/circt.git denied to jpien13. fatal: unable to access 'https://github.com/llvm/circt.git/': The requested URL returned error: 403

Once I get perms I can make PR. Super new to open source and this project so if there are guidelines regarding perms or best practices please let me know! I am also on the dicord as pien1423 if you need to contact me. Thanks!

youngar commented 1 month ago

Hi @jpien13, thanks for working on this! To get around the issue, you should fork circuit, push your branch to your fork, and then open a cross-fork PR from your fork to llvm/circt. You might find this guide helpful. Once you have a some commits under your belt you can ask to be added to the llvm project.

jpien13 commented 1 month ago

@youngar just made the PR! Not sure what the review process is but let me know if you have any feedback :) thanks!