[PATCH] D134859: [clang][Interp] Implement basic support for floating point values

[Joshua Cranmer via Phabricator via cfe-commits]

jcranmer-intel added inline comments.


================
Comment at: clang/lib/AST/Interp/Floating.h:27-29
+  template <unsigned ReprBits> struct Repr;
+  template <> struct Repr<32> { using Type = float; };
+  template <> struct Repr<64> { using Type = double; };
----------------
tbaeder wrote:
> jcranmer-intel wrote:
> > aaron.ballman wrote:
> > > Er, how will this extend to `long double` where the number of bits is rather more difficult?
> > Or `half` and `bfloat`, which are both 16-bit floating-point types?
> I have spent some time with this today and tried to simply always use `APFloat` instead of a primitive type. Unfortunately that doesn't work because what we put on the stack is not the `Floating` (or `Integral`), but the underlying primitive type. So even if we do the final math (in `::add`, etc) via `APFloat`, we need something we can serialize to `char[]` so we can put it on the stack. Do you think that would work?
I don't know enough about the structure of the bytecode interpreter here to say for sure, but this smells to me like you're baking in an assumption that every primitive target type has a corresponding primitive type on the host. This assumption just doesn't hold when it comes to floating point (only two of the seven types, `float` and `double`, are generally portable, and even then, there be dragons in some corner cases).

If you do need to continue down this route, there are two requirements that should be upheld:
* The representation shouldn't assume that the underlying primitive type exists on host (bfloat16 and float128 are better test cases here).
* Conversion to/from host primitive types shouldn't be easy to accidentally do.

(Worth repeating again that bit size is insufficient to distinguish floating point types: `bfloat` and `half` are both 16-bit, PPC `long double` and IEEE 754 quad precision are both 128-bit, and x86 `long double` is 80 bits stored as 96 bits on 32-bit and 128 bits on 64-bit.)


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

https://reviews.llvm.org/D134859