Cuda Performance Metrics/Profiling by thetheodor · Pull Request #198 · facebookresearch/TensorComprehensions

thetheodor · 2018-03-23T14:30:19Z

Add CUPTI-based profiling functionality in CudaRTCFun.

There are several performance metrics (listed here. Each metric requires measuring (possibly multiple types of) hardware events. Since not all events can be measured at the same time, a kernel must be launched multiple times (it's 12 times with the current list of hardcoded metrics).

nicolasvasilache · 2018-04-03T13:46:14Z

+  CUpti_MetricValue value;
+};
+
+using KernelTy = std::function<void(void)>;


KernelType ?

nicolasvasilache · 2018-04-03T13:48:09Z

+  size_t eventNameSize = sizeof(eventName) - 1;
+  TC_CUPTI_CHECK(cuptiEventGetAttribute(
+      eventId, CUPTI_EVENT_ATTR_NAME, &eventNameSize, eventName));
+  eventName[127] = '\0';


eventName[eventNameSize] ?

nicolasvasilache · 2018-04-03T13:48:24Z

+  TC_CUPTI_CHECK(cuptiEventGetAttribute(
+      eventId, CUPTI_EVENT_ATTR_NAME, &eventNameSize, eventName));
+  eventName[127] = '\0';
+  CHECK_GT(eventNameSize, 0);


Move before and also check < 128?

nicolasvasilache · 2018-04-03T13:48:47Z

+    uint64_t sum,
+    uint64_t normalized,
+    uint64_t* values) {
+  char eventName[128];


constexpr blah = 128; ?

nicolasvasilache · 2018-04-03T13:49:33Z

+    }
+  }
+  ss << ')';
+  LOG(INFO) << ss.str();


cleanup plz, maybe it already is cleaned up?

nicolasvasilache · 2018-04-03T13:50:11Z

+
+  ss << eventName << " (normalized) (" << sum << " * " << numTotalInstances
+     << ") / " << numInstances << " = " << normalized;
+  LOG(INFO) << ss.str();


cleanup plz, maybe it already is cleaned up?

nicolasvasilache · 2018-04-03T13:51:30Z

+    TC_CUPTI_CHECK(cuptiEventGroupGetAttribute(
+        group, CUPTI_EVENT_GROUP_ATTR_NUM_EVENTS, &numEventsSize, &numEvents));
+    eventIdsSize = numEvents * sizeof(CUpti_EventID);
+    eventIds = (CUpti_EventID*)malloc(eventIdsSize);


ScopeGuard([]{free(eventIds);});

nicolasvasilache · 2018-04-03T13:54:05Z

+          eventIds[j],
+          &valuesSize,
+          values.data()));
+      CHECK_EQ(numInstances, valuesSize / sizeof(uint64_t));


If these CHECKs are expected to get hit regularly, consider throwing instead

nicolasvasilache · 2018-04-03T14:13:51Z

+  }
+}
+
+uint64_t metricValueAsUint64(const CudaMetric& metric) {


Can you make this function and the next 2 be member methods called as<T> and only instantiated for the proper types?

nicolasvasilache · 2018-04-03T14:22:00Z

+    kernel_();
+  }
+
+  // XXX:this check fails with some combinations of metrics my assumption is


dead code removal, plz

nicolasvasilache · 2018-04-03T14:29:30Z

+    std::vector<const void*> inputs) const {
+  int dev;
+  TC_CUDA_RUNTIMEAPI_ENFORCE(cudaGetDevice(&dev));
+  if (perGpuModule_.count(dev) == 0) {


Factor out and reuse across functions plz

nicolasvasilache · 2018-04-03T14:38:20Z

+// Steal the executor instance and give it back under lock.
+// Run outside of lock on owning ExecutorType.
+template <typename ExecutorType>
+typename ExecutorType::ProfilingInfoType ExecutionEngine<ExecutorType>::profile(


Let's please definitely not copy-paste this piece as it involves tricky concurrent code.
Let's instead factor out the code in another function and pass it a std::function since the only difference seems to be to call run vs profile.

nicolasvasilache · 2018-04-03T14:38:55Z

-  /// data in GPU address space.
-  /// No tensor-related information can be checked so it is the user's
-  /// responsibility to ensure that shapes and strides match.
+  typename ExecutorType::ProfilingInfoType profile(


This is likely going to be non-core team facing API, please document the function

skimo-openhub · 2018-04-03T14:59:57Z

    ${CUDA_curand_LIBRARY}
    ${CUDA_LIBRARIES}
    ${CUDA_NVRTC_LIBRARIES}
+    ${CUDA_cupti_LIBRARY}


CUDA_cupti_LIBRARY doesn't seem to be defined anywhere.

I get

CMake Error: The following variables are used in this project, but they are set to NOTFOUND. Please set them or make sure they are set and tested correctly in the CMake files: CUDA_cupti_LIBRARY (ADVANCED) linked by target "tc_cuda" in directory /home/skimo/git/c2isl/src/core

find_package(cuda) should have set it. Is there a cupti directory under cuda_root_dir/extras in your machine?

skimo-openhub

Make sure 4715b90 gets squashed into the appropriate commit before merging.

skimo-openhub · 2018-04-03T15:40:04Z

On Tue, Apr 03, 2018 at 08:29:43AM -0700, Theodoros Theodoridis wrote: ttheodor commented on this pull request. > target_link_libraries( tc_cuda ${CUDA_CUDA_LIBRARIES} ${CUDA_curand_LIBRARY} ${CUDA_LIBRARIES} ${CUDA_NVRTC_LIBRARIES} + ${CUDA_cupti_LIBRARY} `find_package(cuda)` should have set it. Is there a cupti directory under `cuda_root_dir/extras` in your machine?

No, I don't have that on any of my machines. Do I need to install something extra? Then please update the documentation in this commit. skimo

thetheodor · 2018-04-03T16:07:35Z

You shouldn't have to install it separately (I didn't).

The CUPTI library is supported on all platforms supported by the CUDA Toolkit, and is available on the CUDA Downloads page as part of the CUDA Tools SDK.

nicolasvasilache · 2018-04-04T09:43:05Z

@caffe2bot retest this please

skimo-openhub · 2018-04-04T09:54:16Z

+  } else {
+    CHECK(false) << "Invalid metric value conversion.";
+  }
+}


These functions result in warnings about control reaching
end of non-void function.

The compiler can't figure out that CHECK(false) always leads to program termination. I could return dummy values to make the compiler happy.

Or use something like __builtin_unreachable

skimo-openhub · 2018-04-04T09:57:37Z

On Tue, Apr 03, 2018 at 05:39:57PM +0200, Sven Verdoolaege wrote: On Tue, Apr 03, 2018 at 08:29:43AM -0700, Theodoros Theodoridis wrote: > ttheodor commented on this pull request. > > target_link_libraries( > tc_cuda > > ${CUDA_CUDA_LIBRARIES} > ${CUDA_curand_LIBRARY} > ${CUDA_LIBRARIES} > ${CUDA_NVRTC_LIBRARIES} > + ${CUDA_cupti_LIBRARY} > > `find_package(cuda)` should have set it. Is there a cupti directory under `cuda_root_dir/extras` in your machine? No, I don't have that on any of my machines. Do I need to install something extra?

Installing the package `libcupti-dev` solved the problem for me. skimo

skimo-openhub · 2018-04-04T10:07:49Z

On Wed, Apr 04, 2018 at 09:58:34AM +0000, Theodoros Theodoridis wrote: ttheodor commented on this pull request. > + size_t valueKindSize = sizeof(valueKind); + TC_CUPTI_CHECK(cuptiMetricGetAttribute( + metric.id, CUPTI_METRIC_ATTR_VALUE_KIND, &valueKindSize, &valueKind)); + return valueKind; +} + +double metricValueAsDouble(const CudaMetric& metric) { + auto valueKind = getValueKind(metric); + if (valueKind == CUPTI_METRIC_VALUE_KIND_DOUBLE) { + return metric.value.metricValueDouble; + } else if (valueKind == CUPTI_METRIC_VALUE_KIND_PERCENT) { + return metric.value.metricValuePercent; + } else { + CHECK(false) << "Invalid metric value conversion."; + } +} The compiler can't figure out that `CHECK(false)` always leads to program termination. I could return dummy values to make the compiler happy.

Please do. skimo

skimo-openhub · 2018-04-04T14:39:19Z

  // since we launched only 1 kernel, we should have only 1 kernel record
  TC_CUPTI_CHECK(cuptiActivityGetNextRecord(buffer, validSize, &record));

+#if (CUPTI_API_VERSION >= 10)


This test doesn't seem to match the commit message.
You're doing something special for versions "> 9.0" not "< 9.0".
I don't know which of the two is correct.
However, you seem to be modifying new code, so this should be squashed in.
There is no point in introducing known broken commits.

The commit message refers to CUDA version. The check is on CUPTI version (their versions are tied but they use different numbers).

I'll squash it in the right commit.

skimo-openhub · 2018-04-04T14:47:51Z

On Wed, Apr 04, 2018 at 02:46:15PM +0000, Theodoros Theodoridis wrote: ttheodor commented on this pull request. > @@ -116,7 +116,11 @@ void CUPTIAPI bufferCompleted( // since we launched only 1 kernel, we should have only 1 kernel record TC_CUPTI_CHECK(cuptiActivityGetNextRecord(buffer, validSize, &record)); +#if (CUPTI_API_VERSION >= 10) The commit message refers to CUDA version. The check is on CUPTI version (their versions are tied but they use different numbers).

Thanks. I missed that. skimo

CUPTI is used to query gpu performance counters/events and compute metrics such as occupancy. Not all events can be measured in one run, as such for a particular metric multiple kernel launches might be required. This commit only includes a set of hardcoded metrics (later on these should be selectable).

thetheodor requested review from ftynse and nicolasvasilache March 23, 2018 14:30

facebook-github-bot added the CLA Signed label Mar 23, 2018

prigoyal added the wip label Mar 23, 2018

thetheodor force-pushed the cupti branch from 4ad8011 to a362d1d Compare March 24, 2018 12:16

thetheodor changed the title ~~[RFC] Cuda Profiling~~ Cuda Performance Metrics/Profiling Mar 24, 2018

thetheodor removed the wip label Mar 24, 2018

nicolasvasilache mentioned this pull request Mar 26, 2018

Increase the default number of the autotuner generations in python bindings. #195

Closed

2 tasks

thetheodor force-pushed the cupti branch 2 times, most recently from 67097ab to f7d6902 Compare March 28, 2018 17:16

nicolasvasilache reviewed Apr 3, 2018

View reviewed changes

thetheodor force-pushed the cupti branch 2 times, most recently from e1c97c1 to fc1f2d1 Compare April 3, 2018 14:33

nicolasvasilache reviewed Apr 3, 2018

View reviewed changes

skimo-openhub reviewed Apr 3, 2018

View reviewed changes

skimo-openhub suggested changes Apr 3, 2018

View reviewed changes

thetheodor force-pushed the cupti branch 2 times, most recently from 1b57f34 to 74cdd3e Compare April 3, 2018 15:31

skimo-openhub suggested changes Apr 4, 2018

View reviewed changes

thetheodor force-pushed the cupti branch from 2f0bf92 to 57bfe95 Compare April 4, 2018 13:52

skimo-openhub reviewed Apr 4, 2018

View reviewed changes

thetheodor force-pushed the cupti branch 7 times, most recently from c18f652 to d596872 Compare April 11, 2018 16:04

thetheodor force-pushed the cupti branch 7 times, most recently from c98f779 to 63bc46f Compare April 17, 2018 11:17

thetheodor force-pushed the cupti branch from 63bc46f to ee17496 Compare April 19, 2018 18:02

Theodoros Theodoridis added 7 commits April 25, 2018 13:13

Support CudaProfilingInfo in OptionsCache

851bbe9

Add a profiling path in ExecutionEngine/CudaTcExecutor

b113801

Make cuda profiling optional in the autotuner

490ee55

Make CudaProfiler usable with multiple devices

840251d

Enable profiled runs in test_autotuner

d70afc8

Remove irrelevant metrics

a97df54

thetheodor force-pushed the cupti branch from ee17496 to a97df54 Compare April 25, 2018 11:14

Uh oh!

Conversation

thetheodor commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolasvasilache Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skimo-openhub left a comment

Choose a reason for hiding this comment

Uh oh!

skimo-openhub commented Apr 3, 2018 via email

Uh oh!

thetheodor commented Apr 3, 2018

Uh oh!

nicolasvasilache commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skimo-openhub commented Apr 4, 2018 via email

Uh oh!

skimo-openhub commented Apr 4, 2018 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skimo-openhub commented Apr 4, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thetheodor commented Mar 23, 2018 •

edited

Loading

nicolasvasilache Apr 3, 2018 •

edited

Loading