Halide icon indicating copy to clipboard operation
Halide copied to clipboard

"Update definition 0 of function foo has not been scheduled" warnings should be errors

Open steven-johnson opened this issue 2 years ago • 7 comments

What it says on the tin -- no one follows the warning, not even us, we generate dozens of these during the build. (Don't even ask me how many are generated in google3...)

We should either remove the warning entirely, or (IMHO) declare that this will be an error, not a warning, in the next Halide, and require people to have code that explicitly marks things as "unscheduled".

steven-johnson avatar Dec 06 '23 20:12 steven-johnson

warnings being warnings and not errors is useful while developing a schedule. That particular warning serves as a todo list while working on a schedule, and has saved my ass several times. Perhaps we need a -werror-equivalent target flag?

abadams avatar Dec 06 '23 20:12 abadams

warnings being warnings and not errors is useful while developing a schedule.

And yet we ignored them in lots of our own tests.

I'll probably just change it to an error internal to google.

steven-johnson avatar Dec 06 '23 22:12 steven-johnson

Looks like the PR that reenabled that warning added it in a bunch of places but perhaps not everywhere:

https://github.com/halide/Halide/pull/6602/files

Where are you seeing it in our tests?

There are also the mullapudi autoscheduler warnings that show up in our build logs, which need to just be silenced because they're not actionable.

abadams avatar Dec 06 '23 22:12 abadams

I'll just make a PR where warnings are errors to flush them out

abadams avatar Dec 06 '23 22:12 abadams

Ah, a bunch of our tests do things that we intentionally warn about. E.g. emulating float16 math, or generating assertions that are guaranteed to fail at runtime. So I guess we shouldn't expect our tests to be warning-free.

abadams avatar Dec 06 '23 22:12 abadams

I don't want to bog down in dogma, but IMO compiler warnings are useless as a way to transmit "you really need to pay attention to this" -- far too many coders will simply always ignore them, either because their build system makes it easy to do, or because they are bad coders (sorry, we all know it's true), or because the warnings are compiler-dependent. (IIRC, Golang drew a design line in the sand at the start and said "No warnings, ever").

I don't have a better answer for this sort of thing, but as I'd said elsewhere, i think there are better approaches (eg for emulated fp math, make it an error unless they affirmatively opt-in that they are ok with emulated). This is not the hill I want to die on today, though.

steven-johnson avatar Dec 06 '23 23:12 steven-johnson

In principle I agree, and as a pragmatic matter, I welcome PRs that either upgrade specific warnings to errors or silence them. However, whenever I git grep user_warning, I see a bunch of stuff cases where silencing them or promoting them to errors would make for a worse user experience.

As a concrete example, consider deprecating a feature. With warnings, you can do something like:

  1. The feature works
  2. The feature works, but there's a warning that it will be deprecated
  3. The feature requires an explicit opt-in to use
  4. The feature is deleted.

Saying warnings are errors basically just deletes step 2. It's hard to see how that's an improvement to the user experience.

abadams avatar Dec 06 '23 23:12 abadams