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; };
----------------
aaron.ballman wrote:
> 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?
Going through `APInt` is already possible in `APFloat`, and that might have
some methods to go to char arrays.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134859/new/
https://reviews.llvm.org/D134859
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
