dblaikie 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; }; ---------------- jcranmer-intel wrote: > 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. Yeah - IR gets serialized APFloats somehow (maybe through some layers of indirection) so I'd suggest looking at how, say, a global float named constant gets lowered to bitcode to see how that APFloat is serialized. If that's not enough of a pointer for you/others to find something helpful, I can look further to see what I can find (I got some of the way - a small example with just `extern const float v; const float v = 3.14;` and breaking on APFloat's ctor immediately finds the APFLoat being constructed from that literal - then I looked at llvm::GlobalVariable's ctor, which does break on the creation of `v`, but didn't continue further to figure out how the parsed value gets into the IR constant) 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