mark half(float) constructor as explicit

[ThiagoIze]

As shown in https://github.com/OpenImageIO/oiio/pull/2301#issuecomment-527721367 it's possible right now to write code where a function that takes half is called instead of the (missing) overloaded 32-bit float version. While we might have expected there to be an overload present, sometimes mistakes happen and only the half version is present and when this happens often it will work out well enough that this bug won't be noticed. But performance of course will be lower and sometimes will produce very wrong results as the example above showed -- these can be hard bugs to find and fix.

We could prevent this by making the half(float) constructor explicit, so that it's now a compiler error to try and pass a float or int into a function that expects a half. Admittedly this makes it a little bit more awkward to write certain code and we might then want to add extra explicit constructors, but I think it might be justified by the errors it can prevent.

If you think this is more trouble than it's worth, I understand, but I figured I'd at least throw it out there...

[cary-ilm]

We intend at some point soon to try to rectify the IlmBase Half type with the similar CUDA 16-bit float, and potentially with the upcoming extension of the C++ standard, although that's still a ways out. We'll likely investigate this in more depth then, although requiring explicit conversion would almost certainly break existing application code, and we're reluctant to take that step, although we're sympathetic to the motivation.

If I'm reading it correctly, CUDA's cuda_f16 type provides implicit conversion from both float and double.

[ThiagoIze]

Ideally this would not be explicit and instead the compiler would emit a narrowing conversion warning. This would make a float->half operate in the same way as double->half and give the user a way to detect improper usage. If/when short float makes it to c++ I would hope that most compilers will then emit the narrowing conversion warning. Unfortunately, at the moment I suspect there's no way to support this type of warning.

As for what to do right now, I think we need to factor in the use cases of half:

• For storage, half is a clear win. In this mode I think the explicit conversion might be acceptable since that's only done when writing into the storage, a probably relatively rare operation, and so shouldn't be too painful nor make the code too ugly?
• For high performance computation on x86, half is a bad idea since this generates code that is full of expensive float<->half conversions while the math itself is still done in float. If someone writes the following code, maybe by naively porting over some cuda code that is all half, or worse by accident through templating, I would think we'd want them to be inconvenienced enough that they stop to think about whether this is the best idea. Yes, having to cast each result back to half is ugly, but using half in the first place was a bad idea and this should probably be rewritten to have all the math done in float.

half foo(half h1, half h2, half h3) {
    half a = h1 + h2;
    half b = a + sqrt(h3);
    <imagine 50 more lines of this>
    return some_other_half + b;
}

That's my argument for doing the change. I do understand how breaking backwards compatibility is a problem, so I'm also happy to keep things as is.

[peterhillman]

Perhaps the library could support compiling with something like -DFORCE_HALF_EXPLICIT_CAST

I totally agree it would have been better to have half to float an explicit cast from the beginning for the reasons stated. Changing code to make casts explicit may not be as simple as adding lots of float() to code until it compiles again. Consider a case like this:

namespace SomeThirdPartyLibrary
{
void foo(double d) { }
void foo(float f) { }
}

template<class T> void bar(T input)
{
    SomeThirdPartyLibrary::foo(input);
}

int main()
{
    half h=0;
    double d=0;
    bar(h);
    bar(d);
}

Here bar() calls one of the two overloaded foo() functions based on its type, but foo only has double and float. halfs get implicitly casted to float. Naively adding an explicit float cast to bar would also force double to float. A solution might be to write overloaded wrapper functions to foo with the half version doing an explicit cast to foo(float), and update the wrappers every time the underlying library changes. That might be considered too much of a burden.

I would also worry that making the cast explicit might allow the compiler to discover a weirdly circuitous route for implicitly casting halfs to floats which could be tricky to debug.

[ThiagoIze]

My proposal was only for doing an explicit half (float f);. The other direction operator float () const; stays implicit just like before. That means the code above should continue to work, and it does seem to still compile for me when I try it with the explicit constructor.

[peterhillman]

Indeed only making float-to-half explicit would be less problematic for templated functions, though I suspect could still get you into gotchas. I wonder whether having implicit casts only in one direction would be a problem in itself?

As I say, a compile-time option to enforce explicit casting from float to half seems like a safe compromise, at least for an interim version.