Add roctx ranges to TinyProfiler #2057

jmsexton03 · 2021-05-26T00:45:00Z

Summary

Adds roctx markup similarly to existing nvtx markup.

Additional background

New features are controlled by added compile flag USE_ROCTX. This assumes the location of roctracer/libroctracer64 and roctracer/libroctx64 are similar to other ROCm library installations such as rocrand.

Example run to generate results.json

export HIP_PATH=/path/to/rocm/root
cd amrex/Tests/GPU/CNS/Exec/Sod
make USE_HIP=TRUE TINY_PROFILE=TRUE USE_ROCTX=TRUE USE_MPI=FALSE NO_CONFIG_CHECKING=TRUE
srun -n 1 ${HIP_PATH}/bin/rocprof  --hsa-trace --stats --timestamp on --roctx-trace  ./CNS3d.*.TPROF.HIP.ex inputs

This results.json file can be viewed with a browser as described in the documentation using chrome://tracing/
https://github.com/ROCm-Developer-Tools/rocprofiler/blob/amd-master/doc/rocprof.md#43rd-party-visualization-tools

TODO:
Add CMake support for the new flag
Minimum ROCm version enforcement should be updated (or removed)

Checklist

The proposed changes:

fix a bug or incorrect behavior in AMReX
add new capabilities to AMReX
changes answers in the test suite to more than roundoff level
are likely to significantly affect the results of downstream AMReX users
are described in the proposed changes to the AMReX documentation, if appropriate

…licit

Tools/CMake/AMReXParallelBackends.cmake

Tools/GNUMake/comps/hip.mak

Src/Base/AMReX_GpuDevice.cpp

WeiqunZhang · 2021-05-27T16:17:43Z

On spock, one can also compile with make USE_HIP=TRUE TINY_PROFILE=TRUE USE_ROCTX=TRUE HIP_PATH=/opt/rocm-4.1.0/hip without using NO_CONFIG_CHECKING=TRUE.

WeiqunZhang · 2021-05-27T16:30:01Z

Src/Base/AMReX_GpuDevice.cpp

@@ -343,6 +353,9 @@ Device::Initialize ()
            amrex::Print() << "HIP initialized.\n";
        }
    }
+#if defined(AMREX_USE_ROCTX)
+    roctracer_start();


For some reason, we call profileStart twice in ifdef AMREX_USE_CUDA. I assume that's uncessary. (@maxpkatz) So I suggest we move roctracer_start into profileStart, remove the two calls to profileStart in #ifdef AMREX_USE_CUDA, and put a call to profileStart at the end of Device::Initialize.

I don't know why it's there twice but I agree that we only need one of those two., and it makes sense to put it at the end of Initialize.

I might not be remembering this right, but did we try to put a nvtx region around the Initialization to ID if there was unneeded start-up at some point? Is that why it's in there twice, we tried to move the init back to allow wrapping Init?

There are only calls to Print() between the two calls to cudaProfilerStart.

WeiqunZhang · 2021-05-27T16:32:00Z

Src/Base/AMReX_GpuDevice.cpp

@@ -358,6 +371,11 @@ Device::Finalize ()
    cudaProfilerStop();
 #endif

+#if defined(AMREX_USE_HIP) && defined(AMREX_USE_ROCTX)
+    roctracer_stop();


I suggest we move roctracer_stop to Device::profilerStop, remove the call to cudaProfilerStop above, and call Device::profileStop().

WeiqunZhang · 2021-05-27T16:33:30Z

Src/Base/AMReX_TinyProfiler.cpp

@@ -98,6 +98,12 @@ TinyProfiler::start () noexcept
        }
        nvtxRangePush(fname.c_str());
 #endif
+#if defined(AMREX_USE_HIP) && defined(AMREX_USE_ROCTX)


Might be better to use #elif defined()

WeiqunZhang · 2021-05-27T16:35:37Z

Src/Base/AMReX_TinyProfiler.cpp

@@ -178,6 +184,12 @@ TinyProfiler::stop () noexcept
                amrex::Gpu::Device::synchronize();
            }
            nvtxRangePop();
+#endif
+#if defined(AMREX_USE_HIP) && defined(AMREX_USE_ROCTX)
+        if (device_synchronize_around_region) {


Also we probably should move

if (device_synchronize_around_region) { amrex::Gpu::Device::synchronize(); }

outside the ifdefs so that it's called for dpcpp too.

WeiqunZhang · 2021-05-27T16:38:36Z

Once the rocm issue is resolved on spock, we should remove AMREX_USE_ROCTX to be consistent with how we do things in CUDA.

* Make nvtx, roctx, and roctracer_ext header includes have no quotes * Rearrange profiler calls in Initialize and Finalize * Change to elif and move input-flag controlled Gpu synchronize in TinyProfiler * Remove hip version warning as the latest version should always be used

Tools/CMake/AMReXParallelBackends.cmake

ax3l · 2021-05-27T18:50:10Z

Is there already an apt package that we can install in CI for roctracer?

amrex/.github/workflows/dependencies/dependencies_hip.sh

Line 39 in db8e479

rocm-dev rocrand rocprim

jmsexton03 · 2021-05-27T18:59:06Z

Is there already an apt package that we can install in CI for roctracer?

amrex/.github/workflows/dependencies/dependencies_hip.sh

Line 39 in db8e479

rocm-dev rocrand rocprim

roctracer-dev should be a dependency of rocm-dev, which we already include

Co-authored-by: Axel Huebl <[email protected]>

ax3l · 2021-05-27T19:13:02Z

Ok, feel free to add roctracer-dev explicitly anyway to make the dependency more visible.

Src/Base/AMReX_TinyProfiler.cpp

Tools/CMake/AMReXOptions.cmake

ax3l · 2021-05-27T20:26:55Z

Docs/sphinx_documentation/source/BuildingAMReX.rst

@@ -473,6 +473,8 @@ The list of available options is reported in the :ref:`table <tab:cmakevar>` bel
   +------------------------------+-------------------------------------------------+-------------------------+-----------------------+
   | AMReX_PROFPARSER             |  Build with profile parser support              | NO                      | YES, NO               |
   +------------------------------+-------------------------------------------------+-------------------------+-----------------------+
+   | AMReX_ROCTX                  |  Build with roctx markup profiling support      | NO                      | YES, NO               |


My view is we should simply remove this option in the future once it's fixed on spock.

ax3l · 2021-05-27T20:32:32Z

Tools/GNUMake/comps/hip.mak

@@ -114,6 +115,15 @@ ifeq ($(HIP_COMPILER),clang)
  # rocThrust - Header only
  # SYSTEM_INCLUDE_LOCATIONS += $(ROC_PATH)/rocthrust/include

+  ifeq ($(USE_ROCTX),TRUE)


We could also turn this on by default with GNUmake builds, as soon as HIP is found.

ax3l · 2021-05-27T20:33:58Z

Tools/GNUMake/comps/hip.mak

+  HIPCC_FLAGS += -DAMREX_USE_ROCTX
+  SYSTEM_INCLUDE_LOCATIONS += $(ROC_PATH)/roctracer/include $(ROC_PATH)/rocprofiler/include
+  LIBRARY_LOCATIONS += $(ROC_PATH)/roctracer/lib $(ROC_PATH)/rocprofiler/lib
+  LIBRARIES +=  -Wl,--rpath=${ROC_PATH}/roctracer/lib -lroctracer64 -lroctx64


Don't we add RPATHS automatically these days from the PR I added in #590 ?
We should probably rely on the USE_RPATH GNUmake option for this lib as well, so users can explicitly control RPATHs with a central option alone:

Suggested change

LIBRARIES += -Wl,--rpath=${ROC_PATH}/roctracer/lib -lroctracer64 -lroctx64

LIBRARIES += -lroctracer64 -lroctx64

Tools/CMake/AMReXParallelBackends.cmake

Co-authored-by: Axel Huebl <[email protected]>

Just rely on USE_RPATH instead for GNUmake. Follow-up to #2057

jmsexton03 added 8 commits May 25, 2021 12:30

Test roctx

94f026a

Make roctracer library include depend on USE_ROCTX

870e7a4

Add version checking for ROCm < 4

65589f6

Remove initialize_device Range for ROCm

ddc2098

Add CMake option, include, and link for roctx

8123062

Fix typo, move roctracer_start header, make CMake roctracer links exp…

085d1e2

…licit

Make HIP version check a warning

d04766d

Merge branch 'development' into roctx_ranges

0da605f

jmsexton03 marked this pull request as ready for review May 26, 2021 21:46

jmsexton03 requested review from WeiqunZhang and ax3l May 26, 2021 21:46

ax3l reviewed May 27, 2021

View reviewed changes

Tools/CMake/AMReXParallelBackends.cmake Outdated Show resolved Hide resolved

WeiqunZhang reviewed May 27, 2021

View reviewed changes

Tools/GNUMake/comps/hip.mak Outdated Show resolved Hide resolved

WeiqunZhang reviewed May 27, 2021

View reviewed changes

Src/Base/AMReX_GpuDevice.cpp Outdated Show resolved Hide resolved

WeiqunZhang reviewed May 27, 2021

View reviewed changes

jmsexton03 added 2 commits May 27, 2021 13:32

Add ifdef AMREX_USE_GPU guards on TinyProfiler sync

942a048

ax3l reviewed May 27, 2021

View reviewed changes

Tools/CMake/AMReXParallelBackends.cmake Outdated Show resolved Hide resolved

Update Tools/CMake/AMReXParallelBackends.cmake

444c349

Co-authored-by: Axel Huebl <[email protected]>

Add explicit CI dependency on roctracer and rocprofiler

873ec4f

ax3l reviewed May 27, 2021

View reviewed changes

Tools/CMake/AMReXParallelBackends.cmake Show resolved Hide resolved

WeiqunZhang and others added 3 commits May 27, 2021 14:34

Update Tools/CMake/AMReXOptions.cmake

fe96d13

Co-authored-by: Axel Huebl <[email protected]>

Update Tools/CMake/AMReXParallelBackends.cmake

3b59240

Co-authored-by: Axel Huebl <[email protected]>

Update Src/Base/AMReX_TinyProfiler.cpp

072f368

Co-authored-by: Axel Huebl <[email protected]>

WeiqunZhang approved these changes May 27, 2021

View reviewed changes

WeiqunZhang merged commit 2c2c699 into AMReX-Codes:development May 27, 2021

ax3l added a commit that referenced this pull request May 27, 2021

HIP ROCTX: USE_RPATH Control

7397128

Just rely on USE_RPATH instead for GNUmake. Follow-up to #2057

ax3l mentioned this pull request May 27, 2021

HIP ROCTX: USE_RPATH Control #2064

Merged

5 tasks

WeiqunZhang pushed a commit that referenced this pull request Jun 1, 2021

HIP ROCTX: USE_RPATH Control (#2064)

ecffb6a

Just rely on USE_RPATH instead for GNUmake. Follow-up to #2057

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add roctx ranges to TinyProfiler #2057

Add roctx ranges to TinyProfiler #2057

jmsexton03 commented May 26, 2021 •

edited

Loading

WeiqunZhang commented May 27, 2021

WeiqunZhang May 27, 2021

maximumcats May 27, 2021

kngott May 27, 2021

WeiqunZhang May 27, 2021

WeiqunZhang May 27, 2021

WeiqunZhang May 27, 2021

WeiqunZhang May 27, 2021

WeiqunZhang commented May 27, 2021

ax3l commented May 27, 2021

jmsexton03 commented May 27, 2021

ax3l commented May 27, 2021 •

edited

Loading

ax3l May 27, 2021 •

edited

Loading

WeiqunZhang May 27, 2021

ax3l May 27, 2021

ax3l May 27, 2021 •

edited

Loading

	\| AMReX_ROCTX \| Build with roctx markup profiling support \| NO \| YES, NO \|
	\| AMReX_ROCTX \| Build with roctx markup profiling support \| ON (if HIP) \| YES, NO \|

	LIBRARIES += -Wl,--rpath=${ROC_PATH}/roctracer/lib -lroctracer64 -lroctx64
	LIBRARIES += -lroctracer64 -lroctx64

Add roctx ranges to TinyProfiler #2057

Add roctx ranges to TinyProfiler #2057

Conversation

jmsexton03 commented May 26, 2021 • edited Loading

Summary

Additional background

Checklist

WeiqunZhang commented May 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WeiqunZhang commented May 27, 2021

ax3l commented May 27, 2021

jmsexton03 commented May 27, 2021

ax3l commented May 27, 2021 • edited Loading

ax3l May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ax3l May 27, 2021 • edited Loading

Choose a reason for hiding this comment

jmsexton03 commented May 26, 2021 •

edited

Loading

ax3l commented May 27, 2021 •

edited

Loading

ax3l May 27, 2021 •

edited

Loading

ax3l May 27, 2021 •

edited

Loading