search

ROCm / HIP-CPU

An implementation of HIP that works on CPUs, across OSes.
MIT License
107 stars 19 forks source link

Cmake fixes #16

Closed MathiasMagnus closed 3 years ago

MathiasMagnus commented 3 years ago

This PR fixes a typo in build interface include dirs and deduplicates some login in test and benchmark definitions.

AlexVlx commented 3 years ago

@MathiasMagnus thank you for submitting this, but I believe that streams might've gotten crossed? For example, we don't want to disable Linux & Windows CI testing - perhaps this needs a bit of a cleanup / separation?

MathiasMagnus commented 3 years ago

Yes, apologies, this is still a WIP. CI is only enabled for the master branch and not for working branches. Could we borrow the Draft PR feature of GitHub? I don't think it's enabled for this repo. It works very well on Vcpkg and is useful for messaging that something is half baked.

As a contributor, when I'm opening a PR, there are two buttons, one is regular PR and the other is Draft PR. It's similar to starting the PR name with "WIP:" on GitLab. The UI changes and clearly states it's not meant for merging, but allows feedback from upstream whether they like the design or not.

MathiasMagnus commented 3 years ago

I Linux and Windows and many things to speed up CI runtimes while all tests are supposed to fail building/executing the same way. I intended to turn them back on once the work is done.

AlexVlx commented 3 years ago

Yes, apologies, this is still a WIP. CI is only enabled for the master branch and not for working branches. Could we borrow the Draft PR feature of GitHub? I don't think it's enabled for this repo. It works very well on Vcpkg and is useful for messaging that something is half baked.

As a contributor, when I'm opening a PR, there are two buttons, one is regular PR and the other is Draft PR. It's similar to starting the PR name with "WIP:" on GitLab. The UI changes and clearly states it's not meant for merging, but allows feedback from upstream whether they like the design or not.

I'm afraid that we're not going to have Draft PRs for the time being, since as far as I can gather they're not available for the sort of account that's underpinning this repository. Perhaps at some point in the future, it is a very useful feature.

MathiasMagnus commented 3 years ago

@AlexVlx Requesting review. I have removed some code duplication in test and example code definitions, also finished moving away from manual control of include and link library additions (especially those added through global state) and consolidated picking up dependencies to imported targets. Build flags are also managed through imported target with compiler-specific branches handles through generator expressions instead of script logic.

The only (intended) functional difference is adding /W4 to the MSVC build flags, as other compilers tend to be used in a similar fashion. Let me know if that's too much. There are a few interesting messages on the console:

Segue

FWIW I would completely move away from adding flags via build scripts which aren't essential for the compilation to succeed. I didn't know what was missing from my (professional) life until I came across this tweet. Build scripts (ideally, at least in my ideal world) would define build requirements only (required lang standards, ABI altering reqs, etc.), not build preferences (diagnostic, optimization levels). It may be something worth considering.

One hot topic internally was adding -Werror and /WX to the scripts by default, because we want to compile warning-free at given warning levels. It constantly tripped people over, who were using newer compilers than the ones others CI was using and it made everyone be constantly adding workarounds to warnings others introduced which CI did not alert us to, because CI images usually don't use bleeding edge compilers.

Long story short (IMHO):

AlexVlx commented 3 years ago

@MathiasMagnus thank you again for submitting this and apologies for the long delay in reviewing it. In general LGTM, but there are a few things that would increase tidiness, so please go over the comments.

MathiasMagnus commented 3 years ago

Thanks, will fix the issues once I get there. rocPRIM will soon land experimental HIP-CPU support, currently with many algos failing, just getting everything to compile using MSVC/GCC/Clang. (Some simple tests do pass). Perhaps at one point HIP-CPU can adopt rocPRIM into it's CI for some massive testing.

AlexVlx commented 3 years ago

Thanks, will fix the issues once I get there. rocPRIM will soon land experimental HIP-CPU support, currently with many algos failing, just getting everything to compile using MSVC/GCC/Clang. (Some simple tests do pass). Perhaps at one point HIP-CPU can adopt rocPRIM into it's CI for some massive testing.

Thank you. Regarding rocPRIM, it'll definitely find its way into the CI infrastructure, the more the merrier:)

AlexVlx commented 3 years ago

@MathiasMagnus thank you very much for this and for making the changes (I did a wee bit of tinkering out of pure pedantry, I hope you don't mind), but overall LGTM!

MathiasMagnus commented 3 years ago

No problem. I couldn't quite figure out the style you were looking for based on the textual desc and the root CMakelists.txt. Maybe one day we'll have semantic aware "Clang-tidy" for CMake scripts.