Skip to content

Conversation

@bwasti
Copy link
Contributor

@bwasti bwasti commented Mar 28, 2019

Stack from ghstack:

Differential Revision: D14901379

@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Mar 28, 2019
@bwasti bwasti requested a review from zdevito March 28, 2019 21:24
#include <ATen/ATen.h>
#include <ATen/core/function_schema.h>

#include <torch/csrc/jit/passes/alias_analysis.h>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this might be bad, what do you think @zdevito ?

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.

Looks pretty good. Small nits.

op_(std::make_shared<Operation>(std::move(op))),
options_(std::move(options)) {}

struct Options {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not make this struct part of the Operator class. I want to move away from exposing users directly to the Operator class (and instead have them directly call .op with the op implementation and options). So I think Options should be its own class. Putting it in its own header will help separate out the depenendencies (i.e. the options rely on alias_analysis, but the operator header does not rely directly on it).


struct Options {
Options() {};
using AliasAnalysisFunction = void (AliasDb::*)(Node*);
Copy link
Contributor

Choose a reason for hiding this comment

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

Making AliasAnalysisFunction a pointer rather than an enum doesn't buy us anything here, but it does strongly couple it to the alias_analysis.h header. How about just an enum in a separate header:

enum AliasAnalysisKind {
   CREATOR,
   EXTRACTOR, 
}

struct Options {
};

We can add more wildly customizable versions by patching options when necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. The analyzeFoo functions in alias analysis are not really designed for outside consumption and are probably not clear to users. I think we could have an enum that covers the most common cases:

  • Purely functional (no aliases, no mutation on inputs).
  • View (output aliases input)
  • Free for all (all inputs may be written to, all outputs may alias any input)
  • Look at the schema (reads a user-provided schema, allows people to do whatever fancy thing).

Copy link
Contributor Author

@bwasti bwasti Apr 3, 2019

Choose a reason for hiding this comment

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

reads a user-provided schema

@suo Not sure I follow how this would work

Copy link
Member

@suo suo Apr 3, 2019

Choose a reason for hiding this comment

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

To be clear, you don't have to put all of those options in this PR (just wanted to enumerate some common cases we'll want to add later).

For "Look at the schema"—people defining a custom op can pass a JIT schema if they want; so they should be able to instruct the alias analysis pass to use that schema to populate alias info (similar to how we do with builtin ops).

bwasti added 4 commits April 2, 2019 17:43
…ias analysis passes"

[jit] Add options to Operator to enable registration of alias analysis passes

gh-metadata: pytorch pytorch 18589 gh/bwasti/3/head
…ias analysis passes"

[jit] Add options to Operator to enable registration of alias analysis passes

gh-metadata: pytorch pytorch 18589 gh/bwasti/3/head
…ias analysis passes"

[jit] Add options to Operator to enable registration of alias analysis passes

gh-metadata: pytorch pytorch 18589 gh/bwasti/3/head
…ias analysis passes"

[jit] Add options to Operator to enable registration of alias analysis passes

gh-metadata: pytorch pytorch 18589 gh/bwasti/3/head
bwasti added 2 commits April 5, 2019 16:17
…ias analysis passes"

[jit] Add options to Operator to enable registration of alias analysis passes

gh-metadata: pytorch pytorch 18589 gh/bwasti/3/head
…ias analysis passes"

[jit] Add options to Operator to enable registration of alias analysis passes

gh-metadata: pytorch pytorch 18589 gh/bwasti/3/head
bwasti added 5 commits April 11, 2019 13:38
…ias analysis passes"

[jit] Add options to Operator to enable registration of alias analysis passes

gh-metadata: pytorch pytorch 18589 gh/bwasti/3/head
…ias analysis passes"

[jit] Add options to Operator to enable registration of alias analysis passes

gh-metadata: pytorch pytorch 18589 gh/bwasti/3/head
…ias analysis passes"

[jit] Add options to Operator to enable registration of alias analysis passes

gh-metadata: pytorch pytorch 18589 gh/bwasti/3/head
…ias analysis passes"

[jit] Add options to Operator to enable registration of alias analysis passes

gh-metadata: pytorch pytorch 18589 gh/bwasti/3/head
…ias analysis passes"

[jit] Add options to Operator to enable registration of alias analysis passes

gh-metadata: pytorch pytorch 18589 gh/bwasti/3/head
const std::string& schemaOrName,
Implementation&& implementation) {
Implementation&& implementation,
OperatorOptions options = OperatorOptions()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be exposed in RegisterOperators::op() as well since that's the suggested way to register ops in the docs/tutorials

bwasti added 2 commits April 16, 2019 10:14
…ias analysis passes"

[jit] Add options to Operator to enable registration of alias analysis passes

gh-metadata: pytorch pytorch 18589 gh/bwasti/3/head
…ias analysis passes"

[jit] Add options to Operator to enable registration of alias analysis passes

gh-metadata: pytorch pytorch 18589 gh/bwasti/3/head
@zou3519 zou3519 deleted the gh/bwasti/3/head branch April 17, 2019 20:17
@facebook-github-bot
Copy link
Contributor

@bwasti merged this pull request in 3a031c4.

@suo
Copy link
Member

suo commented Apr 17, 2019

Looks like this broke the build?

zhangguanheng66 pushed a commit to zhangguanheng66/pytorch that referenced this pull request May 6, 2019
…es (pytorch#18589)

Summary:
Pull Request resolved: pytorch#18589
ghimport-source-id: dab203f

Differential Revision: D14901379

Pulled By: bwasti

fbshipit-source-id: d92a497e280f1b0a63b11a9fd8ae9b48bf52e6bf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants