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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: foad, RKSimon, sepavloff, dblaikie, chandlerc.
aaron.ballman 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:
> aaron.ballman wrote:
> > tbaeder wrote:
> > > jcranmer-intel wrote:
> > > > 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.)
> > > Well, is there a way to convert an APFloat to a char[] that would work instead of going to float/double and storing that? The only thing I see in the docs is `convertToHexString()` (and the docs don't mention whether the conversion is lossy). If not, do you think adding such a conversion to `APFloat` and its various implementations is the better way forward?
> > Let's avoid serializing the floats to strings so that we can parse the string to turn it back into a float later; that's going to have poor performance even if we do get all the corner cases correct regarding things like rounding, etc.
> > 
> > `APFloat` does not have any sort of serialization functionality beyond through strings representing the value. I think you'd have to invent such an interface.
> Do you know who I might talk to regrading such an interface, both the implementation as well as general feasibility?
I think there may be at least two ways to do this: use an `APFloat` and put the serialization interfaces there, or use an `APValue` and put the serialization interfaces there.

Because `APFloat` is an ADT in LLVM, I think it should probably go up on Discourse for broader discussion. @chandlerc is still listed as the code owner for ADTs but he's not been active in quite some time. Instead, I would recommend talking to @dblaikie (he's got a good eye for ADT work in general) and @foad, @RKSimon, and @sepavloff as folks who have recently been touching `APFloat`.

`APValue` is a Clang-specific class, and it already has some amount of serialization support, it seems (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/APValue.h#L54). From a quick look, it seems we're already using `APValue` in a reasonable number of places in the interpreter, so it might make sense to use this object consistently to represent all values in the new interpreter?


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

https://reviews.llvm.org/D134859