KEMBAR78
Make operation call accept Stack& instead of Stack* by zhxchen17 · Pull Request #63414 · pytorch/pytorch · GitHub
Skip to content

Conversation

@zhxchen17
Copy link
Contributor

@zhxchen17 zhxchen17 commented Aug 17, 2021

Stack from ghstack:

Summary:
Misuse of raw pointer in here where stack is never nullable.

Test Plan:
compiles.

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: D30375410

Summary:
Misuse of raw pointer in here where stack is never nullable.

Test Plan:
compiles.

Reviewers:

Subscribers:

Tasks:

Tags:

[ghstack-poisoned]
@facebook-github-bot facebook-github-bot added oncall: jit Add this issue/PR to JIT oncall triage queue cla signed labels Aug 17, 2021
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Aug 17, 2021

🔗 Helpful links

💊 CI failures summary and remediations

As of commit 7147a02 (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


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.

@zhxchen17
Copy link
Contributor Author

@zhxchen17 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@zhxchen17 zhxchen17 changed the title [jit] Make operation call accept Stack& instead Stack* [jit] Make operation call accept Stack& instead of Stack* Aug 17, 2021
Summary:
Misuse of raw pointer in here where stack is never nullable.

Test Plan:
compiles.

Reviewers:

Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
@zhxchen17
Copy link
Contributor Author

@zhxchen17 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link

@SplitInfinity SplitInfinity left a comment

Choose a reason for hiding this comment

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

Nice! Straightforward, positive change. I don't know why this was implemented with Stack* in the first place.

Copy link

@SplitInfinity SplitInfinity left a comment

Choose a reason for hiding this comment

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

Requesting changes as per internal discussion

@albanD albanD removed their request for review August 18, 2021 16:13
Summary:
Misuse of raw pointer in here where stack is never nullable.

Test Plan:
compiles.

Reviewers:

Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
@zhxchen17
Copy link
Contributor Author

@zhxchen17 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.


class Operation {
public:
template <typename F,
Copy link
Contributor Author

@zhxchen17 zhxchen17 Aug 18, 2021

Choose a reason for hiding this comment

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

Update: Made this diff API compatible with registration.
Instead of switching over to Stack&, we can just add another constructor for Operation, such that it can take both Stack* and Stack& callback. For Stack* constructor we can make it explicit that it's deprecated, which shouldn't be a hard break for users.

Note that we wrap another level of lambda for Stack* callback to make the implementation uniform, which may have performance overhead but we can take it as a forcing factor for users to switch to Stack& callback. (or it can be inlined, I don't know).

@zhxchen17 zhxchen17 removed the request for review from zhaojuanmao August 18, 2021 20:37
Summary:
Misuse of raw pointer in here where stack is never nullable.

Test Plan:
compiles.

Reviewers:

Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
@zhxchen17
Copy link
Contributor Author

@zhxchen17 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary:
Misuse of raw pointer in here where stack is never nullable.

Test Plan:
compiles.

Reviewers:

Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
@zhxchen17
Copy link
Contributor Author

@zhxchen17 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary:
Misuse of raw pointer in here where stack is never nullable.

Test Plan:
compiles.

Reviewers:

Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
@zhxchen17
Copy link
Contributor Author

@zhxchen17 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary:
Misuse of raw pointer in here where stack is never nullable.

Test Plan:
compiles.

Reviewers:

Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
@zhxchen17
Copy link
Contributor Author

@zhxchen17 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary:
Misuse of raw pointer in here where stack is never nullable.

Test Plan:
compiles.

Reviewers:

Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
zhxchen17 added a commit that referenced this pull request Aug 27, 2021
Pull Request resolved: #63414



Misuse of raw pointer in here where stack is never nullable.

Differential Revision: [D30375410](https://our.internmc.facebook.com/intern/diff/D30375410/)
ghstack-source-id: 136834416
Summary:
Misuse of raw pointer in here where stack is never nullable.

Test Plan:
compiles.

Reviewers:

Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
Summary:
Misuse of raw pointer in here where stack is never nullable.

Test Plan:
compiles.

Reviewers:

Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
Summary:
Misuse of raw pointer in here where stack is never nullable.

Test Plan:
compiles.

Reviewers:

Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
zhxchen17 added a commit that referenced this pull request Aug 29, 2021
Pull Request resolved: #63414



Misuse of raw pointer in here where stack is never nullable.
ghstack-source-id: 136938318

Differential Revision: [D30375410](https://our.internmc.facebook.com/intern/diff/D30375410/)
facebook-github-bot pushed a commit to pytorch/vision that referenced this pull request Aug 30, 2021
Summary:
Pull Request resolved: pytorch/pytorch#63414

Misuse of raw pointer in here where stack is never nullable.
ghstack-source-id: 136938318

Reviewed By: ejguan

Differential Revision: D30375410

fbshipit-source-id: 9d65b620bb76d90d886c800f54308520095d58ee
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in ac99d63.

@facebook-github-bot facebook-github-bot deleted the gh/zhxchen17/12/head branch September 3, 2021 14:19
datumbox pushed a commit to datumbox/vision that referenced this pull request Sep 7, 2021
…#63414)

Summary:
Pull Request resolved: pytorch/pytorch#63414

Misuse of raw pointer in here where stack is never nullable.
ghstack-source-id: 136938318

Reviewed By: ejguan

Differential Revision: D30375410

fbshipit-source-id: 9d65b620bb76d90d886c800f54308520095d58ee
kazhang pushed a commit to pytorch/vision that referenced this pull request Sep 7, 2021
…#63414) (#4380)

Summary:
Pull Request resolved: pytorch/pytorch#63414

Misuse of raw pointer in here where stack is never nullable.
ghstack-source-id: 136938318

Reviewed By: ejguan

Differential Revision: D30375410

fbshipit-source-id: 9d65b620bb76d90d886c800f54308520095d58ee

Co-authored-by: Zhengxu Chen <zhxchen17@fb.com>
@tugsbayasgalan tugsbayasgalan changed the title [jit] Make operation call accept Stack& instead of Stack* Make operation call accept Stack& instead of Stack* Oct 18, 2021
jjsjann123 pushed a commit to jjsjann123/nvfuser that referenced this pull request Oct 29, 2022
Summary:
Pull Request resolved: pytorch/pytorch#63414

Misuse of raw pointer in here where stack is never nullable.
ghstack-source-id: 136938318

Test Plan:
compiles.

Imported from OSS

Reviewed By: ejguan

Differential Revision: D30375410

fbshipit-source-id: 9d65b620bb76d90d886c800f54308520095d58ee
jjsjann123 pushed a commit to jjsjann123/nvfuser that referenced this pull request Nov 10, 2022
Summary:
Pull Request resolved: pytorch/pytorch#63414

Misuse of raw pointer in here where stack is never nullable.
ghstack-source-id: 136938318

Test Plan:
compiles.

Imported from OSS

Reviewed By: ejguan

Differential Revision: D30375410

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

Labels

cla signed 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.

3 participants