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

Reenable warning about unscheduled update definitions #6602

Merged
merged 5 commits into from
Feb 18, 2022

Conversation

abadams
Copy link
Member

@abadams abadams commented Feb 1, 2022

and fix associated issues in the tests and apps.

This is an old warning that stopped triggering because it wasn't tested.
We should either remove it, fix the trigger conditions, or perhaps make
it an error. This PR fixes the trigger conditions and fixes all
instances of the warning in our tests and apps.

The warning triggers if you schedule some but not all of the update
definitions of a Func. It's to protect against the common error of only
scheduling the pure definition of something like a summation. The
warning can be suppressed by inserting a call to func.update(idx).

and fix associated issues in the tests and apps.

This is an old warning that stopped triggering because it wasn't tested.
We should either remove it, fix the trigger conditions, or perhaps make
it an error. This PR fixes the trigger conditions and fixes all
instances of the warning in our tests and apps.

The warning triggers if you schedule some but not all of the update
definitions of a Func. It's to protect against the common error of only
scheduling the pure definition of something like a summation. The
warning can be suppressed by inserting a call to func.update(idx).
@steven-johnson steven-johnson self-requested a review February 2, 2022 17:19
Copy link
Contributor

@steven-johnson steven-johnson left a comment

Choose a reason for hiding this comment

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

Lots of broken stuff, removing approval

@abadams
Copy link
Member Author

abadams commented Feb 2, 2022

Looks like an llvm master breakage, and an uncovered bug in atomics. I'll investigate.

@steven-johnson
Copy link
Contributor

Looks like an llvm master breakage, and an uncovered bug in atomics. I'll investigate.

Could bit be related to https://reviews.llvm.org/D115955 ? (It was reverted last night, I'm working on getting a repro case for them)

@steven-johnson steven-johnson self-requested a review February 3, 2022 23:55
@alexreinking
Copy link
Member

The warning can be suppressed by inserting a call to func.update(idx).

Seeing func.update(idx); as a single line in code looks like a discarded expression that shouldn't have a side-effect. I think it would be nicer to read something more like func.update(idx).schedule_default(); or some other verb (:bike: :derelict_house:) that clearly indicates what is happening.

@steven-johnson
Copy link
Contributor

So where do we stand with this? Do we want to land it as-is and hope to do a proper linter tool in the future? Or what?

@alexreinking
Copy link
Member

Up to Andrew, I guess. My suggestion was just a suggestion.

@alexreinking
Copy link
Member

This looks great! Thanks for indulging me, @abadams 🙂

@alexreinking alexreinking added the release_notes For changes that may warrant a note in README for official releases. label Feb 17, 2022
@steven-johnson
Copy link
Contributor

The Windows failure appears to be a flake, although possibly a real bug (opened issue as #6621). OK to land.

@abadams abadams merged commit be1269b into master Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_notes For changes that may warrant a note in README for official releases.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants