Skip to content

Conversation

@zheng-xq
Copy link
Contributor

@zheng-xq zheng-xq commented Jun 9, 2020

Stack from ghstack:

Differential Revision: D21949849

@zheng-xq zheng-xq requested a review from apaszke as a code owner June 9, 2020 08:30
zheng-xq added a commit that referenced this pull request Jun 9, 2020
ghstack-source-id: cb67584
Pull Request resolved: #39717
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jun 9, 2020
@zheng-xq zheng-xq requested review from ZolotukhinM and nickgg June 9, 2020 08:30
Copy link

@ZolotukhinM ZolotukhinM left a comment

Choose a reason for hiding this comment

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

Overall looks good, but please add some tests for this change.

}
// TODO: fast exp
// TODO: fast erf
// TODO: fast sigmoid

Choose a reason for hiding this comment

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

Nit: remove the finished "TODO" please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

const std::vector<CodeGen::BufferArg>& params,
at::Device device = at::kCPU);

class TORCH_API GenericIntrinsicsExpander : public IRMutator {

Choose a reason for hiding this comment

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

Nit: we're creating an entire class (+ derived from it) just to redefine a single function. We could've get away with just a single free-standing function here.

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 class and function is invoked implicity through accept_mutator. Unless we come up with more magic wrappers here, there is no easy way to call into this in our current infrastructure with a free standing function.

@zheng-xq zheng-xq requested a review from ZolotukhinM June 9, 2020 18:40
zheng-xq added a commit that referenced this pull request Jun 9, 2020
ghstack-source-id: 073bbe5
Pull Request resolved: #39717
Copy link

@ZolotukhinM ZolotukhinM left a comment

Choose a reason for hiding this comment

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

Looks good if the tests are there!

@facebook-github-bot
Copy link
Contributor

@zheng-xq merged this pull request in 6bdfd6a.

@facebook-github-bot facebook-github-bot deleted the gh/zheng-xq/4/head branch June 13, 2020 14:17
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.

4 participants