Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RELAY][GRAD] handle Tuple/TupleGetItem in first order gradient #5946

Merged
merged 2 commits into from Jun 30, 2020

Conversation

t-vi
Copy link
Contributor

@t-vi t-vi commented Jun 27, 2020

This adds tuple support for first order gradients.

We may have tuples passed to or returned from functions and at this stage, they aren't inlined (because we want to compute gradients based on that granularity), limiting the possibility to eliminate Tuple/GetTupleItem.
Examples of ops with tupels include dropout and concatenate.

@t-vi
Copy link
Contributor Author

t-vi commented Jun 27, 2020

@junrushao1994 @MarisaKirisame @tqchen for the friends of gradients...

I have a third thing (fixing an efficiency issue I found), but it also is in gradient.cpp, so I'll send that afterwards.

@tqchen tqchen changed the title handle Tuple/TupleGetItem in first order gradient [RELAY][GRAD] handle Tuple/TupleGetItem in first order gradient Jun 28, 2020
@tqchen
Copy link
Member

tqchen commented Jun 28, 2020

also cc @yzhliu @icemelon9

Copy link
Contributor

@MarisaKirisame MarisaKirisame left a comment

Choose a reason for hiding this comment

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

LGTM

}
return Tuple(res);
} else {
LOG(FATAL) << "unsupported type to zero: " << tt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
LOG(FATAL) << "unsupported type to zero: " << tt;
LOG(FATAL) << "unsupported type to ones: " << tt;

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 goes away with the unification...

@@ -106,14 +106,67 @@ struct ADValueNode {
}
};

Expr MultiZerosType(const Type& t) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you unify MultiZeros with MultiOnes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just pushed that. Thank you for the suggestion.

@MarisaKirisame MarisaKirisame merged commit 957aefb into apache:master Jun 30, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jun 30, 2020
…he#5946)

* handle Tuple/TupleGetItem in first order gradient

* Unify MultiOnes/MultiZeros.
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Jul 2, 2020
…he#5946)

* handle Tuple/TupleGetItem in first order gradient

* Unify MultiOnes/MultiZeros.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants