Glow: Glow doesn't compile anymore due to FP16

[tlepley-cadence]

I'm having issues compiling Glow currently.
I get:

fp16.h:155:33: error: unable to find numeric literal operator 'operator""f'
  const float exp_scale = 0x1.0p-112f;
                                 ^~~~
fp16.h:155:33: note: use -fext-numeric-literals to enable more built-in suffixes

If I add -fext-numeric-literals for g++, I get later an error with clang (this option is not supported by Clang)

    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fext-numeric-literals")
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fext-numeric-literals")

Error:

clang-6.0: error: unknown argument: '-fext-numeric-literals'

[tlepley-cadence]

Note: I'm under Linux:
gcc version 8.1.1 20180502 (Red Hat 8.1.1-1) (GCC)

[nadavrot]

@Maratyszcza @qcolombet

[qcolombet]

Strange, the parsing should be fine as long as you set -std=c++11.
@tlepley-cadence which file is this failing to compile?

[qcolombet]

For the record, our CI use g++ (Ubuntu 4.8.4-2ubuntu1~14.04.3) 4.8.4.

[bertmaher]

-std=gnu++11 seems to work: https://gcc.godbolt.org/z/_QNCTV. I find this surprising.

[tlepley-cadence]

@tlepley-cadence which file is this failing to compile?

It's in thirdparty/fp16/include/fp16/fp16.h

[tlepley-cadence]

-std=gnu++11 seems to work: https://gcc.godbolt.org/z/_QNCTV. I find this surprising.

Very interesting web site !

[qcolombet]

@tlepley-cadence which file is this failing to compile?
It's in thirdparty/fp16/include/fp16/fp16.h

I meant where it was included and thus which file fails to compile (like Float16.cpp, Type.h). I was wondering which command line arguments we might miss and I need the target for that.

[tlepley-cadence]

@qcolombet

/usr/bin/g++  -DGLOW_WITH_CPU=1 -DWITH_PNG=1 -I/home/tlepley/git/glow/include -Iinclude -I. -isystem /home/tlepley/pub/include -I/home/tlepley/git/glow/thirdparty/fp16/include -Wall -Wnon-virtual-dtor -fno-exceptions -fno-rtti -g -fno-omit-frame-pointer -O0 -fPIC -fvisibility=hidden -fvisibility-inlines-hidden   -std=c++14 -MD -MT lib/Base/CMakeFiles/Base.dir/Tensor.cpp.o -MF lib/Base/CMakeFiles/Base.dir/Tensor.cpp.o.d -o lib/Base/CMakeFiles/Base.dir/Tensor.cpp.o -c /home/tlepley/git/glow/lib/Base/Tensor.cpp
In file included from /home/tlepley/git/glow/thirdparty/fp16/include/fp16.h:5,
                 from /home/tlepley/git/glow/include/glow/Support/Float16.h:19,
                 from /home/tlepley/git/glow/include/glow/Base/Type.h:21,
                 from /home/tlepley/git/glow/include/glow/Base/Tensor.h:25,
                 from /home/tlepley/git/glow/lib/Base/Tensor.cpp:17:
/home/tlepley/git/glow/thirdparty/fp16/include/fp16/fp16.h:155:26: error: exponent has no digits
  const float exp_scale = 0x1.0p-112f;
                          ^~~~~~

[qcolombet]

Hmm, I agree with @bertmaher this is really surprising.
It worked fine with g++ 5.5 with -std=c++11, but fails starting with g++ 6.1 with the same option. Starting with this version, one needs to use -std=gnu++11.
This is weird.

[qcolombet]

Also interestingly, if I don't specify the standard (i.e., leave std= out), it works...

[qcolombet]

Hi @Maratyszcza, officially the hex syntax for floating point number is only supported since C++17 according to https://en.cppreference.com/w/cpp/language/floating_literal.

What do you recommend?

[qcolombet]

Confirmed it only starts to appear in the draft of standard for C++17, so either we need to fix the library by not using this syntax or we need to change our requirement from C++11 to GNU++11.

[qcolombet]

@tlepley-cadence, could you give a try to the following patch:

diff --git a/include/fp16/fp16.h b/include/fp16/fp16.h
index be0bb3c..43e605a 100644
--- a/include/fp16/fp16.h
+++ b/include/fp16/fp16.h
@@ -152,7 +152,8 @@ static inline float fp16_ieee_to_fp32_value(uint16_t h) {
         * operate on denormal inputs, and do not produce denormal results.
         */
        const uint32_t exp_offset = UINT32_C(0xE0) << 23;
-       const float exp_scale = 0x1.0p-112f;
+       /*      const float exp_scale = 0x1.0p-112f;*/
+       const float exp_scale = fp32_from_bits(UINT32_C(0xF) << 23);
        const float normalized_value = fp32_from_bits((two_w >> 4) + exp_offset) * exp_scale;

        /*

If that works, that's probably the best way to fix the issue.

[qcolombet]

Interestingly, the hex syntax is part of C99, therefore it seems to me that g++ is pedantic in that case and I guess it explains why it works if we don't set any -std= option.

[nadavrot]

@tlepley-cadence @qcolombet @Maratyszcza I agree let's submit a patch to fix the FP16 library.

[Maratyszcza]

Added a work-around in Maratyszcza/FP16@5e2bd58cc5bfa0738fae08e388eec0ecfba041c4
Checked that the master of FP16 builds fine for -std=c++11 with the most recent gcc.

[nadavrot]

@Maratyszcza Thank you!

[qcolombet]

Hi @Maratyszcza,

Thanks for pushing a fix. Out of curiosity, do you see a need for the complicated macros definition?
Put differently, when using the fp32_from_bits isn’t the compiler smart enough to emit the desired constant?

[tlepley-cadence]

@qcolombet I tried with the commit from @Maratyszcza , and it compiled correctly.

[Maratyszcza]

@qcolombet The version without hex-float literals may cause the constant to be loaded as integer and then transferred to FP register, which comes with a performance penalty compared to loading a constant directly into an FP register.

[qcolombet]

I get that, but I would have expected the compiler do to the right thing
after inlining.
Is it not what you’re seeing?

Le lun. 24 sept. 2018 à 11:39, Marat Dukhan [e-mail] a
écrit :

@qcolombet https://github.com/qcolombet The version without hex-float
literals may cause the constant to be loaded as integer and then
transferred to FP register, which comes with a performance penalty compared
to loading a constant directly into an FP register.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/pytorch/glow/issues/1690#issuecomment-424080690, or mute
the thread
https://github.com/notifications/unsubscribe-auth/APDm1UVRcdi9sOw9fZ87e1BHnQlX8R_Rks5ueSbtgaJpZM4W0Xfv
.

[Maratyszcza]

I didn't look at generated assembly, maybe compilers do the right thing, but I choose to stay on the safe side.

[qcolombet]

Fair enough :)

Le lun. 24 sept. 2018 à 12:13, Marat Dukhan [e-mail] a
écrit :

I didn't look at generated assembly, maybe compilers do the right thing,
but I choose to stay on the safe side.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/pytorch/glow/issues/1690#issuecomment-424091234, or mute
the thread
https://github.com/notifications/unsubscribe-auth/APDm1eArPSpYdN3hPf-qJ5oqszbQ2Vpaks5ueS7AgaJpZM4W0Xfv
.