[PyTorchEdge] promote prim ops by using ops table for mobile runtime by pavithranrao · Pull Request #64816 · pytorch/pytorch

pavithranrao · 2021-09-10T15:01:15Z

Stack from ghstack:

-> [PyTorchEdge] promote prim ops by using ops table for mobile runtime #64816

Differential Revision: D30819926

NOTE FOR REVIEWERS: This PR has internal Facebook specific changes or comments, please review them on Phabricator!

Differential Revision: [D30819926](https://our.internmc.facebook.com/intern/diff/D30819926/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D30819926/)! [ghstack-poisoned]

facebook-github-bot · 2021-09-10T15:01:19Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/64816
📄 Preview docs built from this PR
↩️ [fb-only] Re-run with SSH instructions
🔧 Opt-in to CIFlow to control what jobs run on your PRs

💊 CI failures summary and remediations

As of commit 1a77cb7 (more details on the Dr. CI page):

1/2 failures introduced in this PR
1/2 broken upstream at merge base ddf952b on Sep 10 from 7:19am to 8:34am

1 failure not recognized by patterns:

Job	Step	Action
^{linux-xenial-py3.6-gcc5.4 / build-docs (cpp)}	^Unknown	🔁 rerun

🚧 1 fixed upstream failure:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

If your commit is older than viable/strict, run these commands:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

linux-xenial-cuda11.3-py3.6-gcc7 / test (default, 1, 2, linux.8xlarge.nvidia.gpu) on Sep 10 from 7:19am to 8:34am (88fff22 - 88fff22)
- 🔁 rerun

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 to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

Differential Revision: [D30819926](https://our.internmc.facebook.com/intern/diff/D30819926/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D30819926/)! ghstack-source-id: 137753081 Pull Request resolved: #64816

…le runtime" Differential Revision: [D30819926](https://our.internmc.facebook.com/intern/diff/D30819926/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D30819926/)! [ghstack-poisoned]

Pull Request resolved: #64816 ghstack-source-id: 137952271 Differential Revision: [D30819926](https://our.internmc.facebook.com/intern/diff/D30819926/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D30819926/)!

…le runtime" Differential Revision: [D30819926](https://our.internmc.facebook.com/intern/diff/D30819926/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D30819926/)! [ghstack-poisoned]

Pull Request resolved: #64816 ghstack-source-id: 138035484 Differential Revision: [D30819926](https://our.internmc.facebook.com/intern/diff/D30819926/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D30819926/)!

…le runtime" Differential Revision: [D30819926](https://our.internmc.facebook.com/intern/diff/D30819926/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D30819926/)! [ghstack-poisoned]

…for mobile runtime Pull Request resolved: #64816 ghstack-source-id: 138158141 Differential Revision: [D30819926](https://our.internmc.facebook.com/intern/diff/D30819926/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D30819926/)!

…le runtime" Differential Revision: [D30819926](https://our.internmc.facebook.com/intern/diff/D30819926/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D30819926/)! [ghstack-poisoned]

…for mobile runtime Pull Request resolved: #64816 ghstack-source-id: 138209848 Differential Revision: [D30819926](https://our.internmc.facebook.com/intern/diff/D30819926/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D30819926/)!

iseeyuan

LGTM. Some minor inline comments.

iseeyuan · 2021-09-16T16:46:01Z

torch/csrc/jit/mobile/function.h


 #include <ATen/core/function_schema.h>
 #include <ATen/core/ivalue.h>
+#include <torch/csrc/jit/mobile/prim_ops_registery.h>


If it's only used in function.cpp, it's better to move it to the cpp file.

iseeyuan · 2021-09-16T16:47:17Z

torch/csrc/jit/mobile/prim_ops_registery.h

+std::function<void(Stack&)>& getPrimOpsFn(const std::string& name);
+
+class prim_op_fn_register {
+  std::string prim_ops_name_;


Could you try to remove this quickly and see if it still works?

iseeyuan · 2021-09-16T16:50:56Z

torch/csrc/jit/mobile/promoted_prim_ops.cpp

+    mobile::prim_op_fn_register("prim::Uninitialized", unInitialized),
+    mobile::prim_op_fn_register("aten::to.prim_dtype", toPrimDType),
+    mobile::prim_op_fn_register("prim::is_cuda", isCuda)
+    // TODO: (@pavithran) size is overloaded with int[] and Tensor


It may be some legacy code. There are two operators: aten::size and aten::Size. The only difference is the cap S. You may need to look at which one need to be promoted.

…le runtime" Differential Revision: [D30819926](https://our.internmc.facebook.com/intern/diff/D30819926/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D30819926/)! [ghstack-poisoned]

…for mobile runtime Pull Request resolved: #64816 ## Context: Promoting prim ops: Certain prim ops are frequent than others (like tupleIndex, raiseException, ...). These ops are frequent that they are chosen to be promoted as first class instructions. To promote it requires multiple steps and support from TS team as it changes how the bytecode is serialized and deserialized. So to prevent multiple bytecode version bumps and provided stability while these changes happen, an iterim iterative process is proposed which uses a table to lookup for "promoted" op's function. This allows us to rapidly update the ops list and test on production model without having to change the bytecode. In case of failure, we can quickly revert this change. ## Observation The ops are chosen based on the notebook N1135657 which examines the top frequent ops. ## Fix An iterim solution of having a static table, which when given a prim op name returns a function to be applied on the stack. This helps us check in `function.cpp` to get the "promoted" op. As a fall back, the "promoted" op still resides in `register_prim_ops.cpp` so that the function of prim op is never missed. ghstack-source-id: 138261338 Differential Revision: [D30819926](https://our.internmc.facebook.com/intern/diff/D30819926/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D30819926/)!

facebook-github-bot · 2021-09-17T17:32:30Z

This pull request has been merged in 3c003aa.

facebook-github-bot added cla signed oncall: jit Add this issue/PR to JIT oncall triage queue labels Sep 10, 2021

pavithranrao requested a review from iseeyuan September 16, 2021 02:08

iseeyuan approved these changes Sep 16, 2021

View reviewed changes

facebook-github-bot closed this in 3c003aa Sep 17, 2021

facebook-github-bot added the Merged label Sep 17, 2021

salilsdesai mentioned this pull request Sep 21, 2021

[PyTorch Edge] Revert copying of opname in Function::append_operator #65172

Closed

facebook-github-bot deleted the gh/pavithranrao/18/head branch September 21, 2021 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PyTorchEdge] promote prim ops by using ops table for mobile runtime#64816

[PyTorchEdge] promote prim ops by using ops table for mobile runtime#64816
pavithranrao wants to merge 11 commits intogh/pavithranrao/18/basefrom
gh/pavithranrao/18/head

pavithranrao commented Sep 10, 2021 •

edited

Loading

Uh oh!

facebook-github-bot commented Sep 10, 2021 •

edited

Loading

Uh oh!

iseeyuan left a comment

Uh oh!

iseeyuan Sep 16, 2021

Uh oh!

iseeyuan Sep 16, 2021

Uh oh!

iseeyuan Sep 16, 2021

Uh oh!

facebook-github-bot commented Sep 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

pavithranrao commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

💊 CI failures summary and remediations

1 failure not recognized by patterns:

🚧 1 fixed upstream failure:

Uh oh!

iseeyuan left a comment

Choose a reason for hiding this comment

Uh oh!

iseeyuan Sep 16, 2021

Choose a reason for hiding this comment

Uh oh!

iseeyuan Sep 16, 2021

Choose a reason for hiding this comment

Uh oh!

iseeyuan Sep 16, 2021

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pavithranrao commented Sep 10, 2021 •

edited

Loading

facebook-github-bot commented Sep 10, 2021 •

edited

Loading