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

Add support for not building autoschedulers #6518

Closed
LebedevRI opened this issue Dec 27, 2021 · 13 comments
Closed

Add support for not building autoschedulers #6518

LebedevRI opened this issue Dec 27, 2021 · 13 comments

Comments

@LebedevRI
Copy link
Contributor

LebedevRI commented Dec 27, 2021

Lack of such functionality results in e.g. https://buildd.debian.org/status/fetch.php?pkg=halide&arch=riscv64&ver=13.0.2-2&stamp=1640095586&raw=0

Quoting @alexreinking from chat:

I would gladly accept a PR that adds a Halide_ENABLE_AUTOSCHEDULERS build option. It should be dependent on BUILD_SHARED_LIBS being true and on by default, as it currently is.
Note that it's not trivial since several of the tests assume the autoschedulers exist

@alexreinking
Copy link
Member

Lack of such functionality results in [...]

Can you explain what is happening here, a bit? Scanning through thousands of log lines to determine the relevant parts is not quick.

@LebedevRI
Copy link
Contributor Author

Unhandled exception: Error: The Mullapudi2016 autoscheduler is not supported for the target: riscv-64-linux-no_runtime

@alexreinking
Copy link
Member

I think we should also evaluate why the Mullapudi autoscheduler assert-fails on RISC-V. I'm guessing it wants a CPU target and is missing an option here.

This might be resolved by adding a helper on the target class to distinguish CPUs from accelerators/other targets.

@abadams
Copy link
Member

abadams commented Dec 28, 2021

IMO the assert is bogus and should just be deleted.

@alexreinking
Copy link
Member

IMO the assert is bogus and should just be deleted.

This might actually be a good case for a warning... Mullapudi2016 is not expected to generate good schedules for some targets, but that's not a reason to error out. But we also don't want to leave the opposite impression, right?

@zvookin
Copy link
Member

zvookin commented Dec 28, 2021

Warnings are not a thing.

I can't think of any reason RISC V would be a problem here unless there is some architecture specific data involved, which I don't see on a quick look. (MachineParams is not arch specific, but actual implementation of processor/etc. specific.) If there is some arch specific logic, making RISC V do the same thing as ARM or MIPS is probably fine.

@alexreinking
Copy link
Member

Warnings are not a thing.

I don't know what you mean by this. We have dozens of user_warnings in our codebase. Is there an ongoing effort to remove all of them?

@abadams
Copy link
Member

abadams commented Dec 28, 2021

Each one represents a failure to either make something a hard error or not a problem to begin with.

@abadams
Copy link
Member

abadams commented Dec 28, 2021

But the assert in question doesn't make sense to me. It allows MIPS, and I guarantee this autoscheduler hasn't been tested on MIPS. It should just be removed. The Mullapudi autoscheduler should work fine on any CPU or DSP-like architecture with multiple cores and a cache.

LebedevRI added a commit to LebedevRI/Halide that referenced this issue Dec 28, 2021
As disscussed in halide#6518,
this is a bit dubious, and e.g. prevents building on RISC-V.
@zvookin
Copy link
Member

zvookin commented Dec 28, 2021

Andrew's reply is concise and to the point so I will not feel required to be so :-)

Warnings are basically worthless. Removing them is a good idea but I'm not sure there is an explicit effort to do so. I would however like to avoid adding more as they are a dysfunctional design. A warning is often a compiler engineer saying "this design sucks, and I cannot fix it, so I'm going to add a message to make myself feel better." They can make sense as part of a linter mode where the programmer is explicitly requesting advisory analysis of the program, but in the default mode of compilation, warnings will generally never be seen. And if they are seen, it will almost certainly be annoying rather than helpful.

If the situation is very likely to be a mistake, and there is a straight forward or at least well defined way to write the code that avoids the situation, an error is probably best. Ask if one is programming interactively, will it save time to know this right away or is it more likely to be a possible cleanup for later? If the former, it's an error. If the latter, it's a linter/style/performance-advice thing.

Programming cultures I hang out in generally use "treat all warnings as errors" flags. This requires that every warning be fixable somehow. In extreme cases, a specific warning will be suppressed globally, which is a fairly sure sign that the compiler implementation made a bad decision with that warning. Halide doesn't have a "warnings are errors" flag nor a way to suppress certain warnings largely because we're trying to get rid of the warning thing entirely.

This thread is a good example to consider:

  1. The Mullapudi2016 autoscheduler is almost entirely used as a research comparison base at this point. The person using it is very unlikely to be blindly deploying its output in production.
  2. Andrew and I can't figure out a reason RISC V is second class here in the first place.
  3. Autoscheduling in general comes with no assurance of performance and pretty much always has to be vetted by some other means. We could throw a blanket warning at the start of all of them saying "Hey, might do magic, might pump mud. You've been warned."
  4. If this were a warning, I don't see how one would make it go away. The only way to "fix" it would be for someone to do engineering on the compiler. If this is the case, and we feel it is important for users to acknowledge this situation, we'd want a command line or Target flag to enable experimental support. Thus it would be an error without that flag and nothing with the flag. Generally Halide tends a bit more toward caveat emptor about stuff like this and the autoschedulers are way off in that territory, but as our user base grows, we might consider such experimental feature enable flags.

None of this is intended to cast aspersions on those who thought compiler warnings were an established good design. I've come to the conclusion that they are an antipattern, but only after dealing with them in a number of different build environments and years of experience in the cost/benefit and then having worked on Halide for a while.

In the C/C++ world, a fair number of warnings feel like being in the middle of a battle about the language design. E.g.

    if (a = b) {
    }

is legal by the spec but not legal in a defacto sense because it will generate a warning with most compilers. So we have a world where that code is strictly legal, and in a well defined and straight forward fashion, but it wont compile in a lot of environments people actually use. Perhaps the spec ought to evolve to match practice.

LebedevRI added a commit to LebedevRI/Halide that referenced this issue Dec 28, 2021
As discussed in halide#6518,
this is a bit dubious, and e.g. prevents building on RISC-V,
because there is no way to not build autoschedulers currently.
@LebedevRI
Copy link
Contributor Author

I've gone ahead and drafted up removal of said assertion: #6520

alexreinking pushed a commit that referenced this issue Dec 29, 2021
As discussed in #6518,
this is a bit dubious, and e.g. prevents building on RISC-V,
because there is no way to not build autoschedulers currently.
alexreinking pushed a commit that referenced this issue Jan 5, 2022
As discussed in #6518,
this is a bit dubious, and e.g. prevents building on RISC-V,
because there is no way to not build autoschedulers currently.

(cherry picked from commit 6ed65ba)
alexreinking pushed a commit that referenced this issue Jan 6, 2022
As discussed in #6518,
this is a bit dubious, and e.g. prevents building on RISC-V,
because there is no way to not build autoschedulers currently.

(cherry picked from commit 6ed65ba)
@LebedevRI LebedevRI closed this as not planned Won't fix, can't repro, duplicate, stale Aug 10, 2022
@alexreinking
Copy link
Member

@LebedevRI - why did you close this?

@alexreinking
Copy link
Member

Ah, because the real issue was actually resolved by #6520

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

No branches or pull requests

4 participants