rename nvprof-connector to nvtx-connector, add 3 more hooks #139

cwpearson · 2022-04-18T16:52:05Z

Changes / enhancements to nvprof-connector

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

crtrott · 2022-05-09T20:30:57Z

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 · 2022-05-23T22:22:11Z

@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 · 2023-03-08T04:56:47Z

@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

I would suggest using 'anonymous' rather than the 'anon.' Abbreviation.

vlkale

LGTM. Thanks for these changes. Sorry it was somehow not reviewed until now.

The fix to #147 (now in develop) may relate to or overlap with the discussion on the tools fence callback above.

I will need one other review before we can merge, and I have requested a reviewer.

dalg24

No PR against master. You must target the develop branch.

vlkale · 2023-03-16T17:53:50Z

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

cwpearson · 2023-03-21T15:29:29Z

@dalg24 I have retargeted to develop

vlkale · 2023-03-21T15:31:27Z

@dalg24 I have retargeted to develop

@cwpearson Thanks!

cwpearson · 2023-03-21T15:31:28Z

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 · 2023-03-21T15:51:24Z

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 · 2023-03-21T15:55:12Z

$ 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 · 2023-03-21T15:57:11Z

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

vlkale · 2023-03-21T20:25:43Z

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.

dalg24

When profiling fences, we probably don't want kokkos to fence each time time it calls a callbacks.

kokkos-tools/profiling/roctx-connector/kp_roctx_connector.cpp

Lines 37 to 40 in 0026352

    
           extern "C" void kokkosp_request_tool_settings( 
        
               const uint32_t, Kokkos_Tools_ToolSettings* settings) { 
        
             settings->requires_global_fencing = false; 
        
           }

We probably need a lever to toggle whether or not to enable fence profiling because this changes the behavior of the tool, either marking the full kernel execution or just the launch latency.

dalg24 · 2023-03-21T21:03:54Z

profiling/nvtx-connector/kp_nvtx_connector.cpp

+  if (nullptr == name) {
+    name = "anonymous. Kokkos fence";
+  }


As far as I know this will never occur.

…ew name of nvtx_connector (changing from nvprof_connector).

…nyway

crtrott · 2023-08-10T18:39:11Z

@cwpearson can you review this

crtrott · 2023-08-17T21:46:41Z

profiling/nvtx-connector/kp_nvtx_connector.cpp


 void kokkosp_request_tool_settings(const uint32_t,
                                   Kokkos_Tools_ToolSettings* settings) {
-  settings->requires_global_fencing = false;
+  settings->requires_global_fencing = true;


without this kernel renaming doesn't work, and the regions all only measure kernel launch

dalg24 · 2023-08-17T21:52:19Z

profiling/nvtx-connector/kp_nvtx_connector.cpp

+
+#include "kp_core.hpp"
+
+static int tool_globfences;  // use an integer


Why is it an int and not a bool?

profiling/nvtx-connector/kp_nvtx_connector_domain.h

romintomasetti · 2023-08-18T07:12:35Z

Hi @dalg24 @crtrott !

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

dalg24 · 2023-08-18T22:01:29Z

profiling/nvtx-connector/kp_nvtx_connector.cpp

-static int tool_globfences;  // use an integer
-
+static int tool_globfences;  // use an integer for other options
+using namespace std;


Why? That's generally a bad idea.

Addressed

vlkale requested a review from fnrizzi March 8, 2023 04:57

vlkale reviewed Mar 8, 2023

View reviewed changes

vlkale approved these changes Mar 8, 2023

View reviewed changes

vlkale requested review from dalg24 and removed request for fnrizzi March 8, 2023 05:07

vlkale approved these changes Mar 8, 2023

View reviewed changes

vlkale requested review from crtrott and dalg24 and removed request for dalg24 and crtrott March 9, 2023 22:41

dalg24 requested changes Mar 16, 2023

View reviewed changes

cwpearson force-pushed the nvtx branch 2 times, most recently from e25a7cb to 6d39a56 Compare March 21, 2023 15:25

cwpearson changed the base branch from master to develop March 21, 2023 15:26

cwpearson force-pushed the nvtx branch from 6d39a56 to b3579f1 Compare March 21, 2023 15:27

cwpearson changed the title ~~rename nvproof-connector to nvtx-connector, add 3 more hooks~~ rename nvprof-connector to nvtx-connector, add 3 more hooks Mar 21, 2023

dalg24 previously requested changes Mar 21, 2023

View reviewed changes

crtrott and others added 11 commits August 10, 2023 11:27

Fix global cmakelists to add nvtx directory

c517932

Fixing path to APEX repo, adding Git submodule support for APEX

3e7cb33

Clang-format

ee90a45

putting in filtering of fences when profiling a fence

9b153dc

added KOKKOS_TOOLS_NVTXCONNECTOR_fences

3ab70f7

nvtx connector instead of nvprof connector

d69cd39

put in adapting README for nvtx connector, fixed kp_all.cpp to have n…

6ee452f

…ew name of nvtx_connector (changing from nvprof_connector).

committing with default of tool_globfence to 1.

6183eda

making auto-fencing turned on when environment variable is set to 1

2bf01ac

domain changes

80d4618

Fix up nvtx connector merge

7035fa6

crtrott force-pushed the nvtx branch from 4be3032 to 7035fa6 Compare August 10, 2023 17:58

crtrott added 2 commits August 10, 2023 12:01

nvtx_connector: remove handling of empty string, its a precondition a…

1449cf8

…nyway

Last fixes for nvtx

67b400d

crtrott approved these changes Aug 10, 2023

View reviewed changes

crtrott reviewed Aug 17, 2023

View reviewed changes

dalg24 reviewed Aug 17, 2023

View reviewed changes

Delete unused file and remove unrelated changes to README

1df325c

update connector with limits include to compile on perlmutter

503f66f

dalg24 reviewed Aug 18, 2023

View reviewed changes

vlkale and others added 4 commits August 24, 2023 09:54

take out nvtoolsExt

c2e62da

take out bool logic

1bfc7f5

clang-format

e0dd876

Readd nvToolsExt library for nvtx-connector

5468def

Fix comment

596a820

crtrott force-pushed the nvtx branch from 88976c2 to 596a820 Compare August 24, 2023 17:44

crtrott merged commit bfdfdae into kokkos:develop Aug 24, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rename nvprof-connector to nvtx-connector, add 3 more hooks #139

rename nvprof-connector to nvtx-connector, add 3 more hooks #139

cwpearson commented Apr 18, 2022 •

edited

Loading

crtrott commented May 9, 2022

cwpearson commented May 23, 2022

vlkale commented Mar 8, 2023 •

edited

Loading

vlkale left a comment

vlkale left a comment •

edited

Loading

dalg24 left a comment

vlkale commented Mar 16, 2023

cwpearson commented Mar 21, 2023

vlkale commented Mar 21, 2023

cwpearson commented Mar 21, 2023 •

edited

Loading

vlkale commented Mar 21, 2023

cwpearson commented Mar 21, 2023 •

edited

Loading

cwpearson commented Mar 21, 2023

vlkale commented Mar 21, 2023 via email •

edited

Loading

dalg24 left a comment

dalg24 Mar 21, 2023

crtrott commented Aug 10, 2023

crtrott Aug 17, 2023

dalg24 Aug 17, 2023

romintomasetti commented Aug 18, 2023

dalg24 Aug 18, 2023

	extern "C" void kokkosp_request_tool_settings(
	const uint32_t, Kokkos_Tools_ToolSettings* settings) {
	settings->requires_global_fencing = false;
	}


		#include "kp_core.hpp"

		static int tool_globfences; // use an integer

rename nvprof-connector to nvtx-connector, add 3 more hooks #139

rename nvprof-connector to nvtx-connector, add 3 more hooks #139

Conversation

cwpearson commented Apr 18, 2022 • edited Loading

crtrott commented May 9, 2022

cwpearson commented May 23, 2022

vlkale commented Mar 8, 2023 • edited Loading

vlkale left a comment

Choose a reason for hiding this comment

vlkale left a comment • edited Loading

Choose a reason for hiding this comment

dalg24 left a comment

Choose a reason for hiding this comment

vlkale commented Mar 16, 2023

cwpearson commented Mar 21, 2023

vlkale commented Mar 21, 2023

cwpearson commented Mar 21, 2023 • edited Loading

vlkale commented Mar 21, 2023

cwpearson commented Mar 21, 2023 • edited Loading

cwpearson commented Mar 21, 2023

vlkale commented Mar 21, 2023 via email • edited Loading

dalg24 left a comment

Choose a reason for hiding this comment

dalg24 Mar 21, 2023

Choose a reason for hiding this comment

crtrott commented Aug 10, 2023

crtrott Aug 17, 2023

Choose a reason for hiding this comment

dalg24 Aug 17, 2023

Choose a reason for hiding this comment

romintomasetti commented Aug 18, 2023

dalg24 Aug 18, 2023

Choose a reason for hiding this comment

cwpearson commented Apr 18, 2022 •

edited

Loading

vlkale commented Mar 8, 2023 •

edited

Loading

vlkale left a comment •

edited

Loading

cwpearson commented Mar 21, 2023 •

edited

Loading

cwpearson commented Mar 21, 2023 •

edited

Loading

vlkale commented Mar 21, 2023 via email •

edited

Loading