[JIT] Introduce BuiltinOpFunction and integrate into torchbind #34098

jamesr66a · 2020-03-02T23:00:41Z

Stack from ghstack:

[quantization] Use torchbind for Linear and RNN PackedParams #34140 [WIP][quantization] Use torchbind for Linear PackedParams
[quantization] Make FP16 RNN use new prepack op #34339 [quantization] Make FP16 RNN use new prepack op
[JIT] Introduce BuiltinOpFunction and integrate into torchbind #34098 [JIT] Introduce BuiltinOpFunction and integrate into torchbind
[JIT] Virtualize Function #33921 [JIT] Virtualize Function
[JIT] Move stuff out of class_type.cpp #33900 [JIT] Move stuff out of class_type.cpp

Differential Revision: D20229166

[ghstack-poisoned]

ghstack-source-id: cfaf424 Pull Request resolved: #34098

dr-ci · 2020-03-02T23:10:24Z

💊 CircleCI build failures summary and remediations

As of commit 0ca0eec (more details on the Dr. CI page):

Commit 0ca0eec was recently pushed. Waiting for builds...

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

This comment has been revised 40 times.

zdevito · 2020-03-02T23:09:50Z

aten/src/ATen/core/builtin_function.h

+  }
+
+  void run(Stack& stack) override {
+    auto handle = c10::Dispatcher::singleton().findSchemaOrThrow(


BuiltinOp should just store its Schema, and std::function<void(Stack&)> directly. It does not have to participate in dispatch (and in fact will mostly only incur overhead because of it).

@zdevito - hm, are you sure about that? I'd at least want to have it go through dispatcher to get profiling working. TorchBind ops can be potentially big so it's good to capture them through the same profiling infra. Alternatively - we can just slap the same RecordFunction here too

Also I'm pretty sure someone will want tracing to work too (though it's debatable)

torch/csrc/jit/ir/ir.cpp

torch/csrc/jit/serialization/python_print.cpp

torch/custom_class.h

…bind" [ghstack-poisoned]

…bind" Differential Revision: [D20226171](https://our.internmc.facebook.com/intern/diff/D20226171) [ghstack-poisoned]

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

aten/src/ATen/core/builtin_function.h

dzhulgakov · 2020-03-04T19:21:22Z

torch/custom_class.h

+    classTypePtr->addAttribute("capsule", at::CapsuleType::get());
+
+    c10::getCustomClassTypeMap().insert(
+        {typeid(c10::intrusive_ptr<CurClass>).name(),


do you need demangle here and everywhere? Also - do we need to rely on RTTI for it? you could also use c10::get_fully_qualified_type_name (not sure where exactly we use these names). Or we never use those names other than for accounting? (in which case maybe we should just use typeid directly)

I already asked this in another PR: why do we need demangling? We're simply looking for an exact match. In fact, if demangling is a lossy process, wouldn't that create bugs?

torch/custom_class.h

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

zdevito

This looks like it is going to work. Thanks for all the revisions. I just have some minor things to resolve inline.

aten/src/ATen/core/builtin_function.h

torch/csrc/jit/runtime/interpreter.cpp

zdevito · 2020-03-06T18:01:58Z

torch/custom_class_detail.h

+    bool AllowDeprecatedTypes,
+    size_t... ivalue_arg_indices>
+typename c10::guts::infer_function_traits_t<Functor>::return_type
+call_torchbind_method_from_stack(


This appears to be unused?

It's used in BoxedProxy below

torch/custom_class.h

aten/src/ATen/core/custom_class.cpp

zdevito · 2020-03-06T18:09:01Z

aten/src/ATen/core/ivalue.cpp

  cu_ = std::move(cu);
  type_ = type;
  TORCH_INTERNAL_ASSERT(type_);
-  if (type_->cast<ClassType>()) {


We can possiblly keep this assert if the class table no longer holds StrongTypePtr objects

ivalue::object::create requires a StrongTypePtr for instantiation of any object, and if it's a custom class that's going to be nullptr, so I don't think we can remove this

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

zdevito · 2020-03-06T22:00:21Z

torch/csrc/jit/runtime/interpreter.cpp

-            frames.back().pc = af.pc + 1;
-            enterFrame(code, stack.size() - inst.N);
-            af = ActiveFrame(frames.back());
+            if (!function->isGraphFunction()) {


nit: maybe have runFunction be a function, since this if-statement occurs twice.

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

facebook-github-bot · 2020-03-07T19:08:54Z

@jamesr66a merged this pull request in 45a504d.

[JIT] Introduce BuiltinOpFunction and integrate into torchbind

e55cc6b

[ghstack-poisoned]

jamesr66a requested a review from apaszke as a code owner March 2, 2020 23:00

This was referenced Mar 2, 2020

[JIT] Move stuff out of class_type.cpp #33900

Closed

[JIT] Virtualize Function #33921

Closed

jamesr66a pushed a commit that referenced this pull request Mar 2, 2020

[JIT] Introduce BuiltinOpFunction and integrate into torchbind

05b3ba1

ghstack-source-id: cfaf424 Pull Request resolved: #34098

zdevito reviewed Mar 2, 2020

View reviewed changes

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Mar 2, 2020

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

dad4d4a

…bind" [ghstack-poisoned]

jamesr66a requested a review from zdevito March 3, 2020 17:54

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

8235a20

…bind" [ghstack-poisoned]

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

2857345

…bind" Differential Revision: [D20226171](https://our.internmc.facebook.com/intern/diff/D20226171) [ghstack-poisoned]

jamesr66a mentioned this pull request Mar 3, 2020

[quantization] Use torchbind for Linear and RNN PackedParams #34140

Closed

James Reed added 6 commits March 3, 2020 11:35

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

2f3b214

…bind" Differential Revision: [D20226171](https://our.internmc.facebook.com/intern/diff/D20226171) [ghstack-poisoned]

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

1f74693

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

71c5361

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

f7fb2fe

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

93ef70c

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

22e7e2d

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

jamesr66a requested review from dzhulgakov and suo March 3, 2020 23:11

James Reed added 3 commits March 3, 2020 17:58

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

6280c02

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

52a1bf7

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

8948cb6

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

dzhulgakov reviewed Mar 4, 2020

View reviewed changes

jamesr66a mentioned this pull request Mar 5, 2020

Integrate XNNPACK with custom class for packing weights. #34047

Closed

jamesr66a requested a review from dzhulgakov March 5, 2020 19:33

James Reed added 2 commits March 5, 2020 13:52

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

addf91b

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

60b5a3a

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

jamesr66a mentioned this pull request Mar 6, 2020

[quantization] Make FP16 RNN use new prepack op #34339

Closed

James Reed added 3 commits March 5, 2020 20:54

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

9d0762b

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

162ee7b

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

6f19823

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

zdevito reviewed Mar 6, 2020

View reviewed changes

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

5b9f544

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

jamesr66a requested a review from zdevito March 6, 2020 21:56

zdevito approved these changes Mar 6, 2020

View reviewed changes

Update on "[JIT] Introduce BuiltinOpFunction and integrate into torch…

0ca0eec

…bind" Differential Revision: [D20229166](https://our.internmc.facebook.com/intern/diff/D20229166) [ghstack-poisoned]

facebook-github-bot closed this in 45a504d Mar 7, 2020

facebook-github-bot added the merged label Mar 7, 2020

facebook-github-bot deleted the gh/jamesr66a/203/head branch March 11, 2020 14:16

mruberry added the Merged label Oct 28, 2020

[JIT] Introduce BuiltinOpFunction and integrate into torchbind #34098

[JIT] Introduce BuiltinOpFunction and integrate into torchbind #34098

Uh oh!

Conversation

jamesr66a commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CircleCI build failures summary and remediations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

facebook-github-bot commented Mar 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jamesr66a commented Mar 2, 2020 •

edited

Loading

dr-ci bot commented Mar 2, 2020 •

edited

Loading