-
Notifications
You must be signed in to change notification settings - Fork 397
[NFC][Docs] Update README.md with enhanced build instructions for CIRCT #9203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
While we're at it, we should probably also add the lld linker flag to the readme as it's rough to compiler llvm without it: |
dobios
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't want to make the wording more confusing in the readme, so here are a few suggestions.
Co-authored-by: Amelia Dobis <[email protected]>
Co-authored-by: Amelia Dobis <[email protected]>
|
Thanks @dobios , I applied your suggestions. |
README.md
Outdated
| If you want to only build a specific part, for example the `circt-opt` tool: | ||
| ```sh | ||
| ninja -C build circt-opt | ||
| \``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray backslash
I also suggest taking this comment into account |
|
Fly-by nit but lld isn't in our dependency list so I'm not sure we want it to depend in the default build command - might be worth documenting it as an option separately from the default command? |
@TaoBi22 isn't lld part of llvm? |
It is, but it's not built or installed by the existing command, so someone who doesn't have an existing lld install would still get hit by an opaque error following the instructions |
It's currently part of the "getting started" command though, and linking LLVM with the default linker requires something like 64Gb or memory, so it will crash for most systems without an absurd amount of swap. But yeah if you think it's best to leave it out, your reasoning does make sense |
|
@dobios please help me merge this since it's approved, thanks! |
Hi, this PR add more details about building specific part