aaron.ballman added a reviewer: jcranmer-intel. aaron.ballman added a subscriber: jcranmer-intel. aaron.ballman added a comment.
I've not given this a full review yet, but I do have some comments on a partial review. Mostly, I'm concerned we're going to have different semantics when evaluating than we'd get if the evaluation happened at runtime because we're using the host floating-point environment and not the target. I'm also a bit concerned the design won't extend very easily to `long double` support. We don't have to solve all of this with the initial offering, but I think we want to be sure we're building the float support on a stable base. Adding @jcranmer-intel for another set of eyes on this code from someone who knows many of the most terrifying parts of floating-point support. ================ Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:161 + + return this->emitConstFloat32(E->getValue().convertToFloat(), E); +} ---------------- Is there a reason you want to convert to the host `float` format here instead of passing along the `APFloat` object so semantics follow the target? ================ 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; }; ---------------- Er, how will this extend to `long double` where the number of bits is rather more difficult? ================ Comment at: clang/lib/AST/Interp/Floating.h:74-79 + bool isNegative() const { return V < 0; } + bool isPositive() const { return V >= 0; } + bool isZero() const { return V == 0; } + bool isMin() const { return V == Min; } + bool isMinusOne() const { return V == -1; } + bool isNan() const { return V != V; } ---------------- Hmm, this is making my spidey senses tingle. For example, negative zero should be negative, but `<` won't help you with that: https://godbolt.org/z/xonxqPv5r (and it definitely won't help for negative inf or a signed NaN). 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