rename nvproof-connector to nvtx-connector, add 3 more hooks

[cwpearson]

Changes / enhancements to nvprof-connector

1. rename nvproof-connector to nvtx-connector. nvprof is on its way out, and really this connector is to NVTX anyway.
2. Add hooks for kokkosp_begin_fence, kokkosp_end_fence, and kokkosp_profile_event

[crtrott]

Can you add the code to disable auto fencing by the tool. I think we don't actually want it here. Furthermore, the fence profiling hooks are actually also getting called for the auto-fences, so any tool which profiles fencing needs to deal with that or get tons of false stuff.

[cwpearson]

@crtrott is this the way to disable fencing?

https://github.com/cwpearson/kokkos-tools/blob/ad8848843e6fed0c0c3d76a8689b648eba44f3aa/profiling/nvtx-connector/kp_nvtx_connector.cpp#L60-L62

If so, I think we're good :D

[vlkale]

@crtrott is this the way to disable fencing?

https://github.com/cwpearson/kokkos-tools/blob/ad8848843e6fed0c0c3d76a8689b648eba44f3aa/profiling/nvtx-connector/kp_nvtx_connector.cpp#L60-L62

If so, I think we're good :D

@cwpearson yes, I believe that is right from looking at this a few months ago.

[vlkale]

@dalg24 Thanks. I will work with @cwpearson to fix.

[cwpearson]

@dalg24 I have retargeted to develop

[vlkale]

@dalg24 I have retargeted to develop

@cwpearson Thanks!

[cwpearson]

Interesting, my local clang-format-8 is happy with profiling/nvtx-connector/kp_nvtx_connector.cpp I'm not sure how to resolve the format-check failure.

[vlkale]

Interesting, my local clang-format-8 is happy with profiling/nvtx-connector/kp_nvtx_connector.cpp I'm not sure how to resolve the format-check failure.

Strange. So, the clang-format-8 was added in the last few months, and it should work through the CI in the same way that clang-format-8 works from the LLVM distribution containing clang-format-8. Can you provide the exact version info of clang-format-8? (The Kokkos team has discussed changing the clang-format version in the Kokkos Tools CI from 8 to a higher one like 11, but that's a separate/orthogonal issue).

@dalg24 @crtrott Do you know if there is an easy resolution to the failing format check here given results of CI?

[cwpearson]

$ git log -s | head -n5
commit b3579f18cec82fe7b5dafcfff754823952e61457
Author: Carl Pearson <[email protected]>
Date:   Mon Apr 18 09:49:13 2022 -0700

    rename nvprof-connector to nvtx-connector, add hooks for kokkosp_begin_fence, kokkosp_end_fence, and kokkosp_profile_event
$ git status
On branch nvtx
Your branch is up to date with 'origin/nvtx'.

nothing to commit, working tree clean
$ clang-format-8 --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
$ clang-format-8 -i profiling/nvtx-connector/kp_nvtx_connector.cpp
$ git status
On branch nvtx
Your branch is up to date with 'origin/nvtx'.

nothing to commit, working tree clean

[cwpearson]

It would be great if the format check printed what it thought the diff should be! core and kernels do this

[vlkale]

Thanks. I am a bit baffled here also. I am using version 8.0.1 and this works locally for me too (MacOS). Yes, that would be good if it did print what it should be, and I agree some fixes to format checking process needed for Kokkos Tools. I will circle back soon on this with either a fix to CI process involving clang-format-x or a (temporary) fix to that part of your code to get the format check passed.

[vlkale]

From trying this for another PR, I think this is fixed by including the .clang-format file at the top level of your PR directory (where, e.g., .gitignore, is). I don’t think I see that here?  (This is not obvious and all of that really should be documented in Contributing.md). Can you copy the one from develop into here?

[vlkale]

@.*** *Thanks for these. I will fix, test and get a reviewer for merging.

On Tue, Apr 25, 2023 at 3:23 AM Tomasetti Romin @.***> wrote:

@.**** requested changes on this pull request.

Hi! I went through the code, I think some slight and easy changes are required to go forward ;)

— Reply to this email directly, view it on GitHub https://github.com/kokkos/kokkos-tools/pull/139#pullrequestreview-1399607332, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACZYSIVQVUIXG737RS7LJLLXC6Q2HANCNFSM5TWJZQBQ . You are receiving this because you commented.Message ID: @.***>

[vlkale]

This PR needs to resolve the conflict of the old file of kp_nvprof_connector.cpp on develop. Specifically, the nvtx branch on cwpearson/kokkos-tools needs to be synch'd with develop.

[crtrott]

@cwpearson can you review this

[romintomasetti]

Hi @dalg24 @crtrott !

Reading this PR, it urges us to add proper testing for Cuda, don't you think? :wink: (I mean, the nvtx-connector is compiled, but there is no guarantee that it actually works as intended :crab:)