Halide icon indicating copy to clipboard operation
Halide copied to clipboard

Finish various missing bits of float16/bfloat16 support

Open steven-johnson opened this issue 3 years ago • 6 comments

Fixes issues #3709, #3967, #4916. I think this is the meaningful remaining bits (outside of Python) that have been left unanswered for a while.

Note the addition of halide_float16_t and halide_bfloat16_t in HalideRuntime.h as runtime placeholders that are equivalent to the language-only variants in Float16.h.

Note that there are similar issues in the Python bindings, but I'm going to land those in a subsequent PR to minimize confusion.

steven-johnson avatar Apr 17 '22 23:04 steven-johnson

Something is wonky on ARM systems -- I'm guessing that the addition of the f16/bf16 arguments has somehow confused parameter passing for ARM but haven't nailed it down yet. Investigating.

steven-johnson avatar Apr 18 '22 18:04 steven-johnson

Note that this now deliberately disallows Input<float16_t> and Input<bfloat16_t>, mainly for expediency; it's likely to be rare that using Input instead isn't a simpler and better choice.

(The motivation here is that getting the codegen right on ARM is slightly tricky, in that float16-as-uint would be passed in an integer register, while float16-as-native-arm-type will be passed in an fp register; if all parts of codegen don't agree on the right type, amusing and hard-to-debug crashes can occur. While it's certainly desirable to work out all such flakiness in our code, just on general principle, it's not clear to me that this is likely to be worth the effort at the moment, at least not in comparison to other work to be done.)

steven-johnson avatar Apr 19 '22 01:04 steven-johnson

Gentle review ping

steven-johnson avatar Apr 20 '22 16:04 steven-johnson

Real usage of float16 in Halide that I've seen generally involves a user float16 type, e.g. from some library, and thus requires adding a template instantion for halide_type_of to get it to play nice with things like Halide::cast and Halide::Runtime::Buffer. This is highly non-obvious. The addition of this new runtime type seems to confuse matters further, because you can't actually do anything with it, right? It's just something that has a halide_type_of instantiation? I've been stalling on approving this because I'm wondering if there's something different we can do to help here.

abadams avatar Apr 20 '22 16:04 abadams

Good and fair points. I think I'll drop the PR in its current form and pull out the changes in smaller pieces, probably omitting the new runtime type entirely for now. I do think we need a 'standard' story for runtime types, but I'm not sure what that should look like.

steven-johnson avatar Apr 20 '22 17:04 steven-johnson

...I'd some point I'd really like to circle back and finish this. (Or have someone else finish it.) It's not at all critical, but it bugs me.

steven-johnson avatar Jul 12 '22 22:07 steven-johnson

@zvookin recently pointed me at https://clang.llvm.org/docs/LanguageExtensions.html#half-precision-floating-point, which points out that recent Clang versions have builtin types for these; presumably we should rely on those, falling back to a custom type on other compilers.

steven-johnson avatar Nov 15 '22 22:11 steven-johnson