intel / DML

Intel® Data Mover Library (Intel® DML)
https://intel.github.io/DML/
MIT License
85 stars 17 forks source link

[build] add initial clang support #24

Closed Shamazo closed 1 year ago

Shamazo commented 1 year ago

Primary change is checking for Clang compiler ID. DML_SECURITY_OPTIONS needed to be broken up into link and compile options as clang raises warnings for unused linker flags when compiling with the -c flag. Gtest uses -Werror, which creates errors preventing tests from being built.

Several small bugs also needed to be fixed to compile the core library as well as the examples (mostly clang template deduction). I was unable to get benchmarks to compile due to some quite complex template issues in benchmarks/include/utility.hpp

The contributing.md is a little bit sparse on instructions :). I added myself to the copyright on the files I modified, but happy to do whatever, including signing a CLA, to add clang support. I am working on a clang-only project, and would quite like to use this library.

mzhukova commented 1 year ago

Hi @Shamazo, Thank you for your contribution and interest in DML!

// sorry for confusion with closing PR, this was a misclick on my part

Shamazo commented 1 year ago

"partial" is perhaps a better word. I am unable to get the benchmarks running and currently not very inclined to with my limited time (debugging existing templates is not my strength, but I will add a gist with the build error in case it is obvious to someone else). I am also reluctant to call it full clang support without running benchmarks to compare clang and GCC and validate that clang is not a major perf regression.

I will change up the CMake flags such that tests can be built without benchmarks and then report back on the full test suite probably tomorrow. I am currently unable to validate the hardware mode as I do not yet have access to a sapphire rapids machine. I will test with clang-14 + a skylake machine.

Is there any plan to add CI to the repo? I know adding CI for projects with hardware dependencies is a pain, but it would be nice :). I would volunteer to add basic CI, but cannot offer the hardware resources to do it. (Does Intel have an internal CI for this project or is the default to ask contributors to run tests themselves and confirm that they work?)

mzhukova commented 1 year ago

"partial" is perhaps a better word. I am unable to get the benchmarks running and currently not very inclined to with my limited time (debugging existing templates is not my strength, but I will add a gist with the build error in case it is obvious to someone else). I am also reluctant to call it full clang support without running benchmarks to compare clang and GCC and validate that clang is not a major perf regression.

I will change up the CMake flags such that tests can be built without benchmarks and then report back on the full test suite probably tomorrow. I am currently unable to validate the hardware mode as I do not yet have access to a sapphire rapids machine. I will test with clang-14 + a skylake machine.

Is there any plan to add CI to the repo? I know adding CI for projects with hardware dependencies is a pain, but it would be nice :). I would volunteer to add basic CI, but cannot offer the hardware resources to do it. (Does Intel have an internal CI for this project or is the default to ask contributors to run tests themselves and confirm that they work?)

Yes, there is a plan on adding public CI, but it would be a light-weight scan with partial coverage only. We do have a much broader internal CI, and I (or other engineer) would take your changes and validate it internally for sure, I was just asking about what basic checks were done in order to understand the scope of work for the reviewers.

Testing only host code path is OK for the start, I'll wait on your results before proceeding with reviewing the PR.

Thanks!

Shamazo commented 1 year ago

Hi @mzhukova

All software/auto mode tests pass for me. I added one commit so tests and benchmarks each have their own cmake flag

Here is the build error I get when I try to build the benchmarks with clang 14.

(Force pushes are me fixing up typos)

mzhukova commented 1 year ago

Hi @mzhukova

All software/auto mode tests pass for me. I added one commit so tests and benchmarks each have their own cmake flag

Here is the build error I get when I try to build the benchmarks with clang 14.

(Force pushes are me fixing up typos)

Thanks for the update @Shamazo! I'll look into the benchmarks issue a bit later (merging this PR won't be blocked by this). As for PR review and validation, since we have a full internal CI support and no public CI support yet, I will take your changes/test it out fully internally and update here with the results.

Again, thanks for the contribution!

abdelrahim-hentabli commented 1 year ago

closing this as it changes were implemented as part of d3b19d9010facef70c1f95dfc0d6043d3f96fd01

mzhukova commented 1 year ago

@Shamazo we were able to test out your changes internally and even fixed the benchmarks! Please, let us know if this resolves all your issues with clang build. PR was closed since we committed combined changes (yours and project core team) from our side. Thanks for the contribution!

Shamazo commented 1 year ago

Thank you for merging and figuring out the benchmarks :). If I run into further issues (e.g. on different clang versions) I will open an issue / PR.