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

[opt] Eliminate assertions with non-zero const conditions #877

Merged
merged 1 commit into from
Apr 26, 2020

Conversation

xumingkuan
Copy link
Contributor

@xumingkuan xumingkuan commented Apr 26, 2020

Thanks for @archibate 's idea!
Related PR = #839 (comment)
Before:
benchmark20200426_41fcc24
After:
benchmark20200426_2

WDYT if we optimize a kernel to 0 statements... I set it to 1 statement to avoid division by 0 here. But it's causing too much "improvement" to the geometric mean: nearly 1.02x.

[Click here for the format server]

@xumingkuan
Copy link
Contributor Author

Oh I pushed to the main repo by accident. I think it doesn't matter if I delete this branch after squash and merge...

Copy link
Collaborator

@archibate archibate left a comment

Choose a reason for hiding this comment

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

Cool!

@archibate
Copy link
Collaborator

archibate commented Apr 26, 2020

Btw, could you also perform a test comparing with ti.cfg.check_out_of_bound = False?

@archibate
Copy link
Collaborator

archibate commented Apr 26, 2020

Oh I pushed to the main repo by accident. I think it doesn't matter if I delete this branch after squash and merge...

Congratuation on gaining write-access! We need more discussion on how to make use of it instead of abuse of it. First of all, can a PR author with write-access merge PRs like this on their own without a confirm from the team leader? If we don't make restrictions on review, it will be hard to track BUG; if we restrict too much on review, will definitely increase @yuanming-hu's work as our community grows. We may want to merge some non-critical PRs without the leader's confirm if at least one reviewer approved? If so, write-access members will take more responsibility and their mind should be clear and be cation about mistakes like #879.

Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

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

LGTM!

@yuanming-hu
Copy link
Member

yuanming-hu commented Apr 26, 2020

Oh I pushed to the main repo by accident. I think it doesn't matter if I delete this branch after squash and merge...

Congratuation on gaining write-access! We need more discussion on how to make use of it instead of abuse of it. First of all, can a PR author with write-access merge PRs like this on their own without a confirm from the team leader? If we don't make restrictions on review, it will be hard to track BUG; if we restrict too much on review, will definitely increase @yuanming-hu's work as our community grows. We may want to merge some non-critical PRs without the leader's confirm if at least one reviewer approved? If so, write-access members will take more responsibility and their mind should be clear and be cation about mistakes like #879.

Thanks for thinking about this. I think having one review approval before merging would be good enough.

@xumingkuan
Copy link
Contributor Author

xumingkuan commented Apr 26, 2020

Btw, could you also perform a test comparing with ti.cfg.check_out_of_bound = False?

I'm not sure what you mean. Could you please explain it in more detail? And I think ti.cfg.check_out_of_bound is False by default in release mode?

@xumingkuan xumingkuan merged commit 232e1c1 into master Apr 26, 2020
@xumingkuan xumingkuan deleted the assert1 branch April 26, 2020 17:52
@xumingkuan xumingkuan mentioned this pull request Apr 27, 2020
18 tasks
@archibate
Copy link
Collaborator

Oh I pushed to the main repo by accident. I think it doesn't matter if I delete this branch after squash and merge...

Congratuation on gaining write-access! We need more discussion on how to make use of it instead of abuse of it. First of all, can a PR author with write-access merge PRs like this on their own without a confirm from the team leader? If we don't make restrictions on review, it will be hard to track BUG; if we restrict too much on review, will definitely increase @yuanming-hu's work as our community grows. We may want to merge some non-critical PRs without the leader's confirm if at least one reviewer approved? If so, write-access members will take more responsibility and their mind should be clear and be cation about mistakes like #879.

Thanks for thinking about this. I think having one review approval before merging would be good enough.

Great! Let's merge this then. And later with #839.

@archibate
Copy link
Collaborator

Btw, could you also perform a test comparing with ti.cfg.check_out_of_bound = False?

I'm not sure what you mean. Could you please explain it in more detail? And I think ti.cfg.check_out_of_bound is False by default in release mode?

Oh, did ti.init(debug=True) caused check_oob? Sorry for misunderstanding, seems it's only enabled in test_assert now.

@xumingkuan
Copy link
Contributor Author

The 0 statements was probably due to the mismatch of tests... I can't reproduce it now.

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