Skip to content

Conversation

ZolotukhinM
Copy link

@ZolotukhinM ZolotukhinM commented May 2, 2019

Stack from ghstack:

Differential Revision: D15190193

@raghuramank100
Copy link
Contributor

Feature request:

We will run into cases, where we want to do more than a pattern replacement. Example: Consider replacing a dequant->conv->quant with a qconv. The weight_scale, input_scale and output_scale are nodes in the graph, which we can access directly.

We could (not at present) have a case where we will need to do a computation, requant_scale =round(output_scale/(weight_scale*input_scale)*2^k) and provide this as an input to a qconv(x,w,requant_scale) which would internally do: round(conv(xq,wq)*requant_scale>>k). How do we plan to handle cases like this? Would we have a 'pack/init_conv' op that would do the necessary pre-processing so that the required values (like requant_scale) are available in IR?

TORCH_API std::shared_ptr<script::Module> PatternBasedRewrite(
std::shared_ptr<script::Module> module);

/** \brief A class implementing API for pattern-based subgraph rewrites.
Copy link
Member

Choose a reason for hiding this comment

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

random style nit: I think \brief makes the comment less readable. For doxygen, you can configure so the first line in a javadoc-style comment is automatically taken as the brief description anyway.

So the comment could look like

/**
 * this is the brief description.
 * This is the rest
 */

@ZolotukhinM
Copy link
Author

We will run into cases, where we want to do more than a pattern replacement. Example: Consider replacing a dequant->conv->quant with a qconv. The weight_scale, input_scale and output_scale are nodes in the graph, which we can access directly.
We could (not at present) have a case where we will need to do a computation, requant_scale =round(output_scale/(weight_scale*input_scale)*2^k) and provide this as an input to a qconv(x,w,requant_scale) which would internally do: round(conv(xq,wq)*requant_scale>>k). How do we plan to handle cases like this? Would we have a 'pack/init_conv' op that would do the necessary pre-processing so that the required values (like requant_scale) are available in IR?

There are several options: 1) We could represent this computation directly in IR, in that case everything would just work as-is. 2) This pass is not intended to replace the entire compiler. When there is something it could not reasonably do, we always could add another pass. 3) We could add a preparation pass that would precompute these values and insert the corresponding values into IR.

In the particular case you mentioned, I think we would do (3).

Copy link
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

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

Assuming this is just a rename, I didn't review the code otherwise.

Mikhail Zolotukhin added 2 commits May 7, 2019 10:48
…rewrite."

[JIT] SubgraphRewriter: Rename pattern fusion to subgraph rewrite.

gh-metadata: pytorch pytorch 20082 gh/ZolotukhinM/80/head
…rewrite."

[JIT] SubgraphRewriter: Rename pattern fusion to subgraph rewrite.

gh-metadata: pytorch pytorch 20082 gh/ZolotukhinM/80/head
@zou3519 zou3519 deleted the gh/ZolotukhinM/80/head branch May 8, 2019 18:25
@facebook-github-bot
Copy link
Contributor

@ZolotukhinM merged this pull request in 8a6072c.

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

Labels

Merged module: build Build system issues oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants