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

[Aaron Ballman via Phabricator via cfe-commits]

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