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

Mental notes regarding build system #379

Open
phate opened this issue Jan 31, 2024 · 10 comments
Open

Mental notes regarding build system #379

phate opened this issue Jan 31, 2024 · 10 comments

Comments

@phate
Copy link
Owner

phate commented Jan 31, 2024

  1. I have not thoroughly looked into it, but it seems that libraries are rebuild even when I only change a single test file. Either there is a necessity for it that I am missing, or something is horribly off.
  2. We are struggling to some extend with idiosyncrasies of bash when dealing with configure.sh. I am wondering whether it would be better to rewrite it in python.
  3. The libtooling provides a common interface for implementing individual tools in jlm, and with that has dependencies on all the different jlm libraries. This was done in the old build system to better enable unit tests for the individual tools (and when we did not have "so many" dependencies). The problem is that libtooling depends simply on all the dependencies of every dialect, and with that every tool transitively on all these dependencies. For example, this creates a dependency for jhls to the upcoming MLIR dialect, even though jhls has nothing to do with the MLIR dialect. I am wondering whether we should create tool specific libraries that contain the parts from libtooling for a specific tool? This would still enable the unit tests and also separates the dependencies.
  4. make clean does not seem to clean the build-target/coverage/ folder
  5. Add configure flag for building LLVM dialect.
@phate
Copy link
Owner Author

phate commented Jan 31, 2024

@sjalander @haved @caleridas : Opinions?

@haved
Copy link
Collaborator

haved commented Jan 31, 2024

I agree that there are some weird cases of rebuilding untouched files. I tried to make a reproducible example, but after running make clean and make depend it was no longer reproducible, so there must have been some state somewhere messing it up.

Regarding the configure script, I'm thinking that a rewrite in python could be nice, to have proper argument parsing libraries. Regarding paths with spaces, it seems quite unlikely anyone will ever stumble upon issues with them. Within the Makefile everything is relative, so it would only be an issue if CIRCT_PATH or LLVMCONFIG had spaces. The script could just print a warning if that is the case, and otherwise ignore it.

Separating the tooling files into different libraries could make sense, would this entail splitting up the Command(Line)?.[ch]pp files into separate files?

@phate
Copy link
Owner Author

phate commented Feb 1, 2024

Regarding 1:

This was done on master with HEAD being b4f02a45 (HEAD -> master, origin/master) Fix handling of points-to flags for memcpy in Steensgaard (#373)

// Get a clean state
make clean
make depend

// builds all the libraries and the tools
make all 

vim tests/jlm/llvm/opt/test-cne.cpp // Make trivial change to test file

// Triggers a rebuild of several libllvm library files such as jlm/llvm/backend/jlm2llvm/jlm2llvm.cpp, ...
// Triggers a rebuild of several libhls library files such as jlm/hls/backend/rvsdg2rhls/add-sinks.cpp, ...
// Triggers a rebuild of several (unrelated) test files such as tests/jlm/llvm/frontend/llvm/TestFNeg.cpp, ...
// And then goes on and rebuilds the libraries/tools which depend on the rebuild files above
make all 

I fail the understand why a rebuild of libllvm.a and specifically libhls.a is necessary. Also why it needs to rebuild other test files.

Regarding 2:

  1. I think the bigger problem is the usage of relative paths where absolute paths should be used.
  2. Spaces would be another minor problem.

I was more thinking about the (mental) overhead every time we need to touch this file. I am not bash scripting often enough to keep all the idiosyncrasies in mind, but forgetting "" or something could change the meaning significantly etc. My point is that it is really really easy to make a mistake there. I guess it is not a real big issue, but it was just on my mind.

Regarding 3:
Yes, it would basically mean the split out the specific parts for each tool into its own library, and keep the framework parts in there.

Regarding 4 (which I just sneakily added):
This seems more like an obvious bug to me.

@haved
Copy link
Collaborator

haved commented Feb 1, 2024

As far as I can tell, the .PHONY target is used for only a few of the phony targets in the Makefile, and in one instance there is a target depending on .PHONY, which I've never seen before, and couldn't find explanations for online. It might not be related to the unnecessary compilation you are seeing, as that would be the case if too many targets were marked as phony, and not too few.

@phate
Copy link
Owner Author

phate commented Feb 10, 2024

I am unable to reproduce the problem using the steps mentioned in my last post above.

@haved
Copy link
Collaborator

haved commented Feb 12, 2024

The .PHONY things were fixed by a previous PR, but the header dependency tracking definitely seems too conservative, still. I modify Andersen.hpp, or even just tests/.../TestAndersen.cpp, and suddenly every cpp file in jlm/llvm/ needs to be recompiled when I do make check or make build/tests/jlm/llvm/opt/alias-analyses/TestAndersen.

@haved
Copy link
Collaborator

haved commented Feb 12, 2024

Also as an aside, is there a way of running a single test using the makefile? @caleridas

@caleridas
Copy link
Collaborator

Also as an aside, is there a way of running a single test using the makefile? @caleridas

yes, e.g.: make run-build/tests/jlm/rvsdg/test-cse

@caleridas
Copy link
Collaborator

I agree that there are some weird cases of rebuilding untouched files. I tried to make a reproducible example, but after running make clean and make depend it was no longer reproducible, so there must have been some state somewhere messing it up.

There are unfortunately some limits to automatic dependency detection: when a cpp file is processed all dependencies (including transitive(!) dependencies) are recorded, and this is updated whenever the file itself changes. However if an included header causes a change to a transitive(!) dependency, then the dependency info for the cpp file is not updated. It may be possible to make this correct, but looks rather difficult to me (would need to make it such that changes to dependencies cause recomputation of dependency for cpp file), and TTBOMK no build system is actually doing that (they all rather err on the side of "overcompilation" instead).

@phate
Copy link
Owner Author

phate commented Feb 13, 2024

@haved : We have the target depclean that should clean up the dependencies. Thus, a

> make depclean
> make depend

should hopefully do the trick. I have not really tested it myself yet, as the problem "was gone" recently.

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

No branches or pull requests

3 participants