-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
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).
There was a problem hiding this 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
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) |
…scheduled_stage_warning
Seeing |
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? |
Up to Andrew, I guess. My suggestion was just a suggestion. |
This looks great! Thanks for indulging me, @abadams 🙂 |
The Windows failure appears to be a flake, although possibly a real bug (opened issue as #6621). OK to land. |
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).