ProfileIValue PR by jjsjann123 · Pull Request #585 · csarofeen/pytorch

jjsjann123 · 2020-12-21T19:30:37Z

createConditionalConstant supports profile ivalue including bool, int_list and size
New guard to check conditional constant at runtime
size_eq_guard op to facilitate comparison of dynamic sizes
sum_to_size & _grad_sum_to_size added in integration

profile size added;
sum_to_size and _grad_sum_to_size are working properly;
Add test for backwards;
Address upstream review comments

…or nvfuser. We tried to go around PR pytorch#47667 refactor profiling optional, since upstream is still working on it at this time.

…ze_PR_2

kevinstephano · 2020-12-29T01:15:14Z

+        jit_o = t_jit(x, y)
+        # (TODO) check executed kernel, should extend autograd.profiler to fused
+        # kernels
+        torch.cuda.profiler.start()


Did you mean to leave in cuda profiling?

Doesn't profiler start stop no-op when we don't run it with nvprof/nsys?
But this is a good catch. I was using this to verify kernel executed. There's no good reason to keep them. I'll remove it.

kevinstephano · 2020-12-29T01:31:20Z

+  const int64_t leading_dims = root.size() - sum_to_size.size();
+
+  // Generate reduction axes for leading dims
+  std::vector<int> reduce_dims(leading_dims);


It looks like you might be mixing int64_t and int, is that okay? Is this a situation where size_t cannot be used?

std::vector is needed because of our signature here:

pytorch/torch/csrc/jit/codegen/cuda/arith.h

Line 123 in 9038bb4

const std::vector<int>& reduction_axes,

I thinks int is pretty safe, given that we are dealing dimensions here, instead of size. So unlikely that we are dealing anything larger than 8.

kevinstephano · 2020-12-29T01:32:55Z

+        )[0].graph
+        FileCheck().check(FUSION_GUARD).run(bwd_graph)
+
+        # update shape


What is the second shape testing?

This is mostly trying to verify that we still can handle dynamic shape without recompilation.
I'll put a comment there.

kevinstephano · 2020-12-29T20:08:02Z

          Node* in_const =
-              subgraph.createClone(input->node(), [](Value*) -> Value* {
-                throw std::runtime_error("unexpected input");
+              subgraph.createClone(input->node(), [&](Value* v) -> Value* {


Are you sure you want to capture the entire environment of the lambda function by reference?

We are only capturing pointers; 2. The lambda here is called before the function is returned.
We should be good here.

kevinstephano · 2020-12-29T20:08:37Z

-            graph->createClone(n->input(1)->node(), [](Value*) -> Value* {
-              throw std::runtime_error("unexpected input");
-            });
+        const auto map_inputs = [&](Value* v) -> Value* {


Another place where the entire environment is captured...

kevinstephano · 2020-12-29T20:13:17Z

+      fusion->removeInput(offset);
+
+      // step b. remove the extra dependency inside fusion;
+      for (auto use : fusion_graph->inputs()[offset]->uses()) {


Another possible place where use maybe should be a reference?

kevinstephano · 2020-12-29T20:45:23Z

  void (*fn_run_n_s_)(const Node*, Stack&) = nullptr;
  void (*fn_fuse_graph_)(std::shared_ptr<Graph>&) = nullptr;
  bool (*fn_can_fuse_n_)(const Node*) = nullptr;
+  void (*fn_insert_profile_inodes_)(ProfilingRecord* pr) = nullptr;


This might be an extreme nitpick but shouldn't the variable names not have the _ suffix given they are public data members? (You can choose to ignore :-) )

My interpretation of the google style is that the suffix rule only differs between class and struct, but it irrelevant to access level. https://google.github.io/styleguide/cppguide.html#Variable_Names
I am aware that other style guidelines do put suffix based on that. 🤷

_ suffix is only for private data members (the Google style definition is a bit confusing since it assumes only public members on structs and only private on classes - but the suffix reflects the access level, not struct vs. class)

kevinstephano · 2020-12-29T20:59:49Z

+        const Node* node = n->input(1)->node();
+        // propagate profiled none through other profile_ivalue nodes;
+        while (!profiled_none_flag && node->kind() == prim::profile_ivalue) {
+          profiled_none_flag |= profiled_none_.count(node->input(0));


Are you sure you mean bitwise OR here? It looks like a logical OR.

Oops, good catch 😝

kevinstephano · 2020-12-29T21:06:59Z


 namespace {

+static const auto sizeAttr = Symbol::attr("profiled_size");


Isn't it redundant to use static in an anonymous namespace?

kevinstephano · 2020-12-29T21:18:30Z

+      insertProfileIValue(pr, n, offset);
+    }
+
+    for (auto ib : n->blocks()) {


Are you sure want to copy the block?

blocks() returns an at::ArrayRef<Block*> so it's a trivial copy.

kevinstephano

LGTM

jjsjann123 added 10 commits December 15, 2020 01:47

This is a cherry-pick from upstream PR pytorch#47668 profile ivalue f…

7a05929

…or nvfuser. We tried to go around PR pytorch#47667 refactor profiling optional, since upstream is still working on it at this time.

optional is broken

dc57302

everything is working, but now we are hitting scheduling issue

3455ccf

fixing stupid bug :(

04d9289

removing debug print; addresssing upstream review comments

625a062

Merge remote-tracking branch 'csarofeen/20_12_3_devel' into sum_to_si…

d63f7a6

…ze_PR_2

added test

04ddb04

clang-tidy & clang-format

bd091ac

clean code

4aa7652

flake8

9038bb4

jjsjann123 changed the title ~~[WIP] Sum to size pr 2~~ ProfileIValue PR Dec 29, 2020

fix profile_optional for _grad_sum_to_size

b62ef56

kevinstephano reviewed Dec 29, 2020

View reviewed changes

jjsjann123 added 3 commits December 30, 2020 04:53

review comments

6a8775d

more comments

b9b3d80

review comment

1631a98

kevinstephano approved these changes Jan 5, 2021

View reviewed changes

jjsjann123 merged commit 97d5d76 into 20_12_3_devel Jan 5, 2021

csarofeen deleted the sum_to_size_PR_2 branch June 9, 2021 13:40


		namespace {

		static const auto sizeAttr = Symbol::attr("profiled_size");

Conversation

jjsjann123 commented Dec 21, 2020 • 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!

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!

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!

kevinstephano left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jjsjann123 commented Dec 21, 2020 •

edited

Loading