Skip to content
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

Improve build system #5543

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

MinetaS
Copy link
Contributor

@MinetaS MinetaS commented Aug 19, 2024

<commit description not ready yet>

No functional change

@MinetaS
Copy link
Contributor Author

MinetaS commented Aug 19, 2024

Moved checklist/tests to external (gitbook) pages.

  1. Rewrite Makefile
  2. Source code modification

@MinetaS MinetaS force-pushed the BetterBuild branch 3 times, most recently from 1ecf690 to fefb0a3 Compare September 1, 2024 19:58
@Disservin
Copy link
Member

Since this is getting quite large now, can we try to have certain cleanups be their own pr? Like some of makefile changes which are unrelated to the network architecture restructure please?

@Disservin
Copy link
Member

We discussed the usage of “find” in the past and I think it wasn’t very portable.. I think on windows find already exists and the makefile for me took that find over the posix find because of how the environment variables were set by default

@MinetaS
Copy link
Contributor Author

MinetaS commented Sep 2, 2024

Since this is getting quite large now, can we try to have certain cleanups be their own pr? Like some of makefile changes which are unrelated to the network architecture restructure please?

Yes I 100% agree, I'm now trying to reduce any excessive changes that are not directly related to the primary purpose.

But I don't understand what you meant by "some of makefile changes", can you elaborate? This essentially requires complete rewrite of Makefile at least - and I can't find which are unrelated yet.

@MinetaS
Copy link
Contributor Author

MinetaS commented Sep 2, 2024

Maybe moving out net.sh can be a separate PR? If that's the case I'll prepare one.

@Disservin
Copy link
Member

Disservin commented Sep 2, 2024

But I don't understand what you meant by "some of makefile changes", can you elaborate?

The usage of find would be one thing, though id like to avoid having that altogether.
Then there are a few make rule changes which I don't think are strictly tied to this pr in general, like

profileclean: Removed unnecessary entries.

what are unnecessary entries?

format: Now it informs user when clang-format is not installed.

help: The whole text is exported to an environment variable. It has help-arch as a dependency if ARCH is properly set.

what is the reason behind this?

.depend: Shows message when dependency is updated and Make is restarting.

could be standalone as well no?

etc...

@MinetaS
Copy link
Contributor Author

MinetaS commented Sep 2, 2024

The usage of find would be one thing, though id like to avoid having that altogether.

find substitutes for the previous way of listing source/header files. I thought it was clean and simple rather than manually tracking every single file with long path such as arch/XXX/nnue/YYY.h. Also msys find has higher priority in msys shell on Windows. I don't know if we keep supports for other Unix compatible layers and terminals (i.e. Cygwin, Windows Terminal), but if so I'll revert them back.

what are unnecessary entries?

gcc saves profiling data in profdir so *.gcda and *.gcno are unnecessary. bench.txt is not created anywhere. For args and lt files, I haven't observed such files are created (maybe I missed), I'll check them again.

what is the reason behind this?

format uses clang-format but it is often not included in clang package. I think it could help users to know which to install exactly, but no big reason.

Speaking of help, current Makefile calls echo per line, which is very slow on Windows due to the overhead of CreateProcess (no fork). printf also works, there is no difference.

could be standalone as well no?

When running a rule of CXX_REQUIRED_RULES, it prints which type of compiler is used at the very beginning. When dependency is updated Make restarts itself and prints the same info twice. It is also UX stuff to help users understand why sometimes duplicate message is shown, especially after doing make clean.

Edit: to sum up, some are not strictly tied as you said, such as profileclean or clang-format, but others are still related as I see.

Nothing is final, I always appreciate any type of feedbacks.

@vondele
Copy link
Member

vondele commented Sep 2, 2024

The usage of find would be one thing, though id like to avoid having that altogether.

find substitutes for the previous way of listing source/header files. I thought it was clean and simple rather than manually tracking every single file with long path such as arch/XXX/nnue/YYY.h. Also msys find has higher priority in msys shell on Windows. I don't know if we keep supports for other Unix compatible layers and terminals (i.e. Cygwin, Windows Terminal), but if so I'll revert them back.

for devs having some unrelated files in the tree (test.cpp comes to mind), the usage of find might be problematic. I think it is good to list explicitly.

Generally, I agree with Disservin here, splitting things out that could be merged before makes sense to keep the diff between this branch and master small, which will eventually be helpful I think, for you to maintain it until it is ready, and for later review.

@Disservin
Copy link
Member

Disservin commented Sep 2, 2024

for devs having some unrelated files in the tree (test.cpp comes to mind), the usage of find might be problematic. I think it is good to list explicitly.

for reference we discussed that here, https://github.com/official-stockfish/Stockfish/pull/4790#:~:text=View%20reviewed%20changes-,src/Makefile,-Outdated needs to be expanded though.

In my dev env on windows I have the msys stuff in my env and use the normal windows shell where the windows find has a higher presence, and I'm not sure fishtest requires msys as the shell.

@Disservin
Copy link
Member

Disservin commented Sep 2, 2024

The echo change would be good to have I think, people have already complained about how slow make is on windows.

@MinetaS
Copy link
Contributor Author

MinetaS commented Sep 2, 2024

Generally, I agree with Disservin here, splitting things out that could be merged before makes sense to keep the diff between this branch and master small, which will eventually be helpful I think, for you to maintain it until it is ready, and for later review.

Yes indeed, although I was struggling to find out which can be splitted, especially in Makefile. Soon I'll open some PRs where possible, and some lines Disservin pointed out.

@Disservin
Copy link
Member

Disservin commented Sep 10, 2024

Why do you use i386 as the name for the 64 bit targets?

Okay I just saw it's not limited to 64 bits, it's also 32 bit.. but I still I find that name weird

Just x86 maybe?

@Disservin
Copy link
Member

But I think I like where this is going. The function syntax for the arch implementations do tend to be a bit complex but i think that’s probably okay otherwise we would need to alias them with macros.. I also think that this makes it easier for us to have a binary with different simd code paths in future as well ?

@MinetaS
Copy link
Contributor Author

MinetaS commented Sep 10, 2024

Why do you use i386 as the name for the 64 bit targets?

Okay I just saw it's not limited to 64 bits, it's also 32 bit.. but I still I find that name weird

Just x86 maybe?

Followed gcc convention. Planned to change to x86 later.

@MinetaS
Copy link
Contributor Author

MinetaS commented Sep 10, 2024

But I think I like where this is going. The function syntax for the arch implementations do tend to be a bit complex but i think that’s probably okay otherwise we would need to alias them with macros.. I also think that this makes it easier for us to have a binary with different simd code paths in future as well ?

I think it's time to do some cleanup. Submitted a PR here: #5584

Not desirable changes I guess

@MinetaS
Copy link
Contributor Author

MinetaS commented Sep 20, 2024

Added PowerPC paths and build presets for now.

In my opinion, I think it's better to remove all non-x86/arm presets since we do not have full target arch support (i.e. no AltiVec implementation for ppc family, only popcount and prefetch), and let users compile and build native. In this proposal, the generic implementation calls builtin popcount functions (prefetch can be done in the same way) and compilers would definitely handle it efficiently.

* Restore Bitboard::pretty()
* Replace PEXT-related constexpr stuffs with USE_PEXT macro
Disservin added a commit that referenced this pull request Oct 31, 2024
The Makefile is notoriously slow on windows, because of new processes
being spawned I believe. This pr improves it a little bit for the help
and config-sanity targets, with the latter also improving `make -j
build` because it depends on that. On the same machine ubuntu (wsl) is
more than 3 times faster, if there are other improvements we can make
I'd be happy to hear about them. Ultimately
#5543 also aims to
improve this I believe, but it will take some additional time before
that lands.

```
make config-sanity:

    patch: 6.199s
    master: 12.738s

make help:

    patch: 3.1s
    master: 11.49s

make -j build:

    patch: 36s
    master: 43.25s

make -j build:

    master ubuntu: 10s
```

closes #5642

No functional change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants