Skip to content

Conversation

@bhosmer
Copy link

@bhosmer bhosmer commented Aug 3, 2020

Stack from ghstack:

This PR adds IValue support for the Quantizer class, by adding a QuantizerType tag and associated constructors and destructors.

Notes:

  1. This eliminates the final type unsupported by our boxed kernel wrappers, so here we also take the opportunity to eliminate the artifacts of our previous incomplete support: a) the special profiling entry points that would box a "can't box" string for unsupported types, and b) the metaprogramming to generate an error-raising wrapper for kernels with Quantizer in their signatures.

  2. ConstQuantizerPtr still has some special logic associated with it in the JIT code, commented as temporary; this PR leaves that undisturbed, but should facilitate its elimination in a followup.

Differential Revision: D22894190

[ghstack-poisoned]
@bhosmer bhosmer requested a review from apaszke as a code owner August 3, 2020 06:01
bhosmer pushed a commit that referenced this pull request Aug 3, 2020
ghstack-source-id: f97a929
Pull Request resolved: #42438
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Aug 3, 2020
@bhosmer bhosmer requested a review from smessmer August 3, 2020 06:06
@dr-ci
Copy link

dr-ci bot commented Aug 3, 2020

💊 CI failures summary and remediations

As of commit 4bae4cd (more details on the Dr. CI page):



🚧 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)

Since your merge base is older than viable/strict, run these commands:

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

Check out the recency history of this "viable master" tracking branch.


ci.pytorch.org: 1 failed


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

See how this bot performed.

This comment has been revised 21 times.

static torch::jit::Stack boxArgs(Args... args) {
// TODO Reuse stack vector instead of allocating?
torch::jit::Stack stack;
stack.reserve(sizeof...(Args));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this reserve be called inside torch::jit::push? Maybe other call sites would benefit from it too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I can't think of a downside... OTOH I'm not sure I want to colocate that change with the other stuff in this PR 😁

This PR adds IValue support for the `Quantizer` class, by adding a `QuantizerType` tag and associated constructors and destructors. 

Notes:
1. This eliminates the final type unsupported by our boxed kernel wrappers, so here we also take the opportunity to eliminate the artifacts of our previous incomplete support: a) the special profiling entry points that would box a "can't box" string for unsupported types, and b) the metaprogramming to generate an error-raising wrapper for kernels with `Quantizer` in their signatures.

2. `ConstQuantizerPtr` still has some special logic associated with it in the JIT code, commented as temporary; this PR leaves that undisturbed, but should facilitate its elimination in a followup.

Differential Revision: [D22894190](https://our.internmc.facebook.com/intern/diff/D22894190)

[ghstack-poisoned]
bhosmer pushed a commit that referenced this pull request Aug 4, 2020
ghstack-source-id: 03519e8
Pull Request resolved: #42438
@bhosmer bhosmer requested a review from suo August 4, 2020 02:22
This PR adds IValue support for the `Quantizer` class, by adding a `QuantizerType` tag and associated constructors and destructors. 

Notes:
1. This eliminates the final type unsupported by our boxed kernel wrappers, so here we also take the opportunity to eliminate the artifacts of our previous incomplete support: a) the special profiling entry points that would box a "can't box" string for unsupported types, and b) the metaprogramming to generate an error-raising wrapper for kernels with `Quantizer` in their signatures.

2. `ConstQuantizerPtr` still has some special logic associated with it in the JIT code, commented as temporary; this PR leaves that undisturbed, but should facilitate its elimination in a followup.

Differential Revision: [D22894190](https://our.internmc.facebook.com/intern/diff/D22894190)

[ghstack-poisoned]
bhosmer pushed a commit that referenced this pull request Aug 4, 2020
ghstack-source-id: 8f30266
Pull Request resolved: #42438
This PR adds IValue support for the `Quantizer` class, by adding a `QuantizerType` tag and associated constructors and destructors. 

Notes:
1. This eliminates the final type unsupported by our boxed kernel wrappers, so here we also take the opportunity to eliminate the artifacts of our previous incomplete support: a) the special profiling entry points that would box a "can't box" string for unsupported types, and b) the metaprogramming to generate an error-raising wrapper for kernels with `Quantizer` in their signatures.

2. `ConstQuantizerPtr` still has some special logic associated with it in the JIT code, commented as temporary; this PR leaves that undisturbed, but should facilitate its elimination in a followup.

Differential Revision: [D22894190](https://our.internmc.facebook.com/intern/diff/D22894190)

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

@bhosmer merged this pull request in feeb515.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants