Skip to content

Conversation

eellison
Copy link
Contributor

@eellison eellison commented Jan 2, 2020

Stack from ghstack:

If we know that two constants are the same object, we can ignore other constraints and pool them together. This fixes an issue introduced by the other PR where quantization relied on constant pooling happening for correctness.

Differential Revision: D19269499

@eellison eellison requested a review from apaszke as a code owner January 2, 2020 23:28
eellison pushed a commit that referenced this pull request Jan 2, 2020
ghstack-source-id: f32b021
Pull Request resolved: #31800
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jan 2, 2020
@eellison eellison changed the title check for object equality in constant pooling [JIT] check for object equality in constant pooling Jan 2, 2020
Copy link
Contributor

@jerryzh168 jerryzh168 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 to me

@kostmo
Copy link
Member

kostmo commented Jan 2, 2020

💊 CircleCI build failures summary and remediations

As of commit da840cd:

None of the build failures appear to be your fault.

  • 1/1 broken upstream at merge base c66ca74 since Jan 08

    You may want to rebase on the viable/strict branch (expand for instructions)

    If your commit is newer than viable/strict, you can try basing on an older, stable commit:

    git fetch origin viable/strict
    git rebase --onto viable/strict $(git merge-base origin/master HEAD)
    

    If your commit is older than viable/strict:

    git fetch origin viable/strict
    git rebase viable/strict
    

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

Detailed failure analysis

One may explore the probable reasons each build failed interactively on the Dr. CI website.

🚧 1 upstream failure recognized by patterns:

These builds matched patterns, but were probably caused by upstream breakages:


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.

This comment has been revised 17 times.


// if both values are the same object, we do not need to worry about
// changing the aliasing relationship
bool same_idenity =
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/same_idenity/same_identity/

If we know that two constants are the same object, we can ignore other constraints and pool them together. This fixes an issue introduced by the other PR where quantization relied on constant pooling happening for correctness.

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

[ghstack-poisoned]
eellison pushed a commit that referenced this pull request Jan 3, 2020
ghstack-source-id: eb881f1
Pull Request resolved: #31800

c10::optional<IValue> toIValue(const Value* v) {
if (v->node()->kind() != prim::Constant) {
if (v->node()->kind() != prim::Constant || v->type()->cast<FunctionType>()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there constant Function nodes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea

If we know that two constants are the same object, we can ignore other constraints and pool them together. This fixes an issue introduced by the other PR where quantization relied on constant pooling happening for correctness.

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

[ghstack-poisoned]
eellison pushed a commit that referenced this pull request Jan 3, 2020
ghstack-source-id: d4975b9
Pull Request resolved: #31800
If we know that two constants are the same object, we can ignore other constraints and pool them together. This fixes an issue introduced by the other PR where quantization relied on constant pooling happening for correctness.

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

[ghstack-poisoned]
eellison pushed a commit that referenced this pull request Jan 8, 2020
ghstack-source-id: d19d00a
Pull Request resolved: #31800
@facebook-github-bot
Copy link
Contributor

@eellison merged this pull request in 8ecd3f7.

@facebook-github-bot facebook-github-bot deleted the gh/eellison/41/head branch January 12, 2020 15:17
wuhuikx pushed a commit to wuhuikx/pytorch that referenced this pull request Jan 30, 2020
Summary:
Pull Request resolved: pytorch#31800

If we know that two constants are the same object, we can ignore other constraints and pool them together. This fixes an issue introduced by the other PR where quantization relied on constant pooling happening for correctness.

Test Plan: Imported from OSS

Differential Revision: D19269499

Pulled By: eellison

fbshipit-source-id: 9d4396125aa6899cb081863d463d4f024135cbf4
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.

6 participants