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

Further Makefile updates #52

Merged
merged 5 commits into from
Jul 20, 2023
Merged

Conversation

mosullivan93
Copy link
Contributor

@mosullivan93 mosullivan93 commented Jun 26, 2023

I am mostly using GNU make at the moment, so I eventually found a few other things I thought might be worth changing. Feel free to disagree, e.g. if you think some of the changes could make maintaining the file problematic in the future.

An overview of the commits:

  1. 5041188: Improve modularity of GCC compatibility resolution
    • This uses a Makefile macro (test_cxx_flag) to allow checking an arbitrary flag of the GCC C++ compiler. AVX512-FP16 can be disabled without immediately falling back to march=icelake-client.
    • There is the potential to intelligently downgrade the target architecture further, but that might make removing the affected tests a bit more difficult (e.g. Could potentially handle this with preprocessor directives).
  2. d0623a7: Substantially update Makefile to improve coverage
    • This commit makes significant changes to the design of the Makefile so that it is more sensitive to changes in the other files. Ultimately it's not as smart as meson when it comes to determining which files need compilation, but with the current configuration it wouldn't detect that recompilation is needed when a header is changed inside the tests or benchmarks folders.
    • I opted to use the builtin implicit rules for compiling .o files that are found in prerequisites in order to reduce the number of rules and custom recipes. This mostly relies on stuffing all the options into CXXFLAGS instead.
    • I made the test and benchmark objects depend on the .hpp and .h files in their respective folders, too. This is in addition to their dependence on the files in the src and utils directories. This fits within the new approach by using rules to modify the compiler flags depending on which folder the target resides in.
    • Designated the targets that do not produce a file (with the same name as the rule's target) as phony to ensure they can always be run as expected.
    • Giving consideration to the potential for a user to override CXXFLAGS, the critical changes are now made in an override clause. I also swapped the variables from recursively expanded to simple where it was possible.
  3. 18d036f: Robustify CXX detection logic
    • The conditional assignment on the first line of the Makefile seems like it will actually never fire because CXX holds g++ by default.
    • Therefore, I've added some new checks to (attempt to) intelligently detect and set the CXX variable when the user hasn't already done so. The detected version will also propagate to meson when that is the target.
  4. c0a99e0 (bugfix for 18d036f): Silence GCC discovery and bail when none available
    • On some systems, which will print to stderr when an argument can't be found. This will now be suppressed.
    • Also removed basename to more gracefully handle the situation where g++ is not on the path.

This is a minor update that uses more sophisticated methods to validate
the supported compiler arguments.
The header files are now considered when make decides what targets need
updating. Configured .DEFAULT_GOAL and .PHONY for the non-file targets.
Tweaked file to predominantly rely on the default implicit rules for
building the `.o` objects (read: moved all the options into CXXFLAGS).

In addition to tracking with each `.hpp` or `.h` file inside of tests
and benchmarks (accordingly), each `.o` object will also trigger a
rebuild when there are changes to the main source files in src. However,
each `.cpp` is otherwise independent of each other.
The first line wasn't doing anything previously because CXX is defined
as `g++` by default. This change means that now when CXX is undefined or
empty it can be detected automatically. Additionally, the variable is
marked as exported so that the detected version will be passed on to
meson if it is being invoked via make.
On some systems, `which` will print to stderr when an argument can't be
found. This will now be suppressed. Also removed `basename` to more
gracefully handle the situation where `g++` is not on the path.
Copy link
Contributor

@r-devulap r-devulap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for taking care of the Makefile! :)

@r-devulap r-devulap merged commit f22807a into intel:main Jul 20, 2023
3 checks passed
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.

2 participants