Skip to content

Require asserts for AD tests using the debug-only flag #27824

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

Merged
merged 1 commit into from
Oct 22, 2019

Conversation

asuhan
Copy link
Contributor

@asuhan asuhan commented Oct 21, 2019

No description provided.

@asuhan asuhan requested a review from dan-zheng October 21, 2019 23:57
Copy link
Contributor

@dan-zheng dan-zheng left a comment

Choose a reason for hiding this comment

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

Thanks, nice catch! (If we had CI running tests with no-assertions, I think this error could've been caught earlier.)

@dan-zheng
Copy link
Contributor

@swift-ci Please clean test tensorflow

@bgogul
Copy link
Contributor

bgogul commented Oct 22, 2019

Thanks, nice catch! (If we had CI running tests with no-assertions, I think this error could've been caught earlier.)

Do we even need --debug-only=differentiation for the test?

@dan-zheng
Copy link
Contributor

dan-zheng commented Oct 22, 2019

Thanks, nice catch! (If we had CI running tests with no-assertions, I think this error could've been caught earlier.)

Do we even need --debug-only=differentiation for the test?

Sadly, yes we do, for the purpose of checking AD-generated structs and enums.

During one of the recent merges, structs and enums were changed to be implicit (as they should), meaning they no longer show up via any dumping (e.g. -emit-sil). So, I added LLVM_DEBUG print statements to print the structs and enums via -debug-only=differentiation.

We could add a separate -Xllvm flag just for printing the data structures, but I think reusing -debug-only=differentiation is fine. Other SILOptimizer tests also use -debug-only to FileCheck various properties.

@dan-zheng dan-zheng merged commit 56e4de8 into tensorflow Oct 22, 2019
@asuhan asuhan deleted the asuhan/require_asserts branch October 22, 2019 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants