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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
