[PATCH] D113107: Support of expression granularity for _Float16.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D113107#3620014 <https://reviews.llvm.org/D113107#3620014>, @zahiraam wrote:

> In D113107#3615372 <https://reviews.llvm.org/D113107#3615372>, @zahiraam wrote:
>
>> In D113107#3606106 <https://reviews.llvm.org/D113107#3606106>, @rjmccall wrote:
>>
>>> In D113107#3606094 <https://reviews.llvm.org/D113107#3606094>, @zahiraam wrote:
>>>
>>>> In D113107#3605797 <https://reviews.llvm.org/D113107#3605797>, @rjmccall wrote:
>>>>
>>>>> I think on balance the right thing to do is probably to add an alternative to `-fexcess-precision`, like `-fexcess-precision=none`.  We can default to `-fexcess-precision=standard` and treat `-fexcess-precision=fast` as an alias for `standard` for now.
>>>>
>>>> In https://gcc.gnu.org/onlinedocs/gcc-4.8.2/gcc/Optimize-Options.html#index-ffloat-store-900 ,  it looks like when compiling C, the default is -fexcess-precision=standard which would align with this implementation and our default too. So I think we could use the same name for the option. 
>>>> -fexcess-precision=none corresponds to the current behavior.
>>>> -fexcess-precision=standard = -fexcess-precision=fast corresponds to this implementation.
>>>> Agreed?
>>>
>>> Since you're not landing this option right now anyway, do you mind broaching this with the GCC folks, just to be good neighbors?  You can just say that (1) Clang is looking for a way to request operation-by-operation lowering, (2) it feels like `-fexcess-precision` is the right option to add that to, (3) we don't want to tread on toes by adding an alternative to "their" option without talking to them first, and (4) what do they think about "none"?
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106117
>
> In GCC using no -fexcess-precision is the same than using -fexcess-precision=standard. The patch we are proposing here is implementing this part.  So for  a + b + c, we are generating (_Float16) [((float a) + (float b)) + (float c)]
> In GCC using -fexcess-precision=16 is generating yet another flavor of the operation-by-operation emulation. For the same addition than above, gcc is generating (_Float16) [(float) (_Float16) ((float a) + (float b)) + (float c)]. That's different than our current implementation.
> See https://godbolt.org/z/aM8fzTsj1
> Am I understanding correctly? @pengfei you are interested in the -fexcess-precision=16 part of this right? @rjmccall what do yo think?

As I understand it, `-fexcess-precision=16` is what the current implementation (not this patch) does.

I think you have GCC's default wrong.  As documented, their default depends on the language mode.  In GNU C modes (e.g. `gnu89`) and C++, the default is `-fexcess-precision=fast`, which basically does arbitrary promotion in the optimizer / backend without necessarily honoring casts or assignments.  In standards-compatible C modes (e.g. `c89`), the default is `-fexcess-precision=standard`, which honors casts and assignments as forcing the use of semantic precision for that value.  GCC implements `-fexcess-precision=standard` the same way this patch does: by directly promoting and truncating values in a special emitter in their translation from AST to GIMPLE (a vaguely LLVM-like representation).

The three takeaways for me from that GCC thread (and research I did related to it) are:

- GCC already has an option to disable excess precision arithmetic, which it calls `-fexcess-precision=16`.  So we should at least honor that spelling.  I think we could also justify introducing `-fexcess-precision=none` as an alias, from what I saw in the discussion.
- We should make sure that we set `FLT_EVAL_METHOD` appropriately in any mode that enable excess precision arithmetic.
- We need to make sure that constant evaluation does the same thing that we're doing in IRGen, because the C standard requires frontends that use excess precision to use at least as much precision in constant evaluation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113107/new/

https://reviews.llvm.org/D113107