[PATCH] D137858: ConstantFolding: Constant fold some canonicalizes

[Joshua Cranmer via Phabricator via llvm-commits]

jcranmer-intel added inline comments.


================
Comment at: llvm/include/llvm/IR/Type.h:167-170
+  /// Return true if this is a well-behaved IEEE-like type, which has a IEEE
+  /// compatible layout as defined by isIEEE(), and does not have unnormal
+  /// values
+  bool isIEEELikeFPTy() const {
----------------
I'm not sure I like the name "IEEELike", but, admittedly, I'm struggling to keep up with anything better (as bfloat isn't an IEEE type at all, and x86_fp80 is an IEEE extended precision format).

The documentation comment should clarify that it's referring to `APFloat::isIEEE()`. I think "does not have unnormal values" can be clarified as "does not have non-IEEE values, such as x86_fp80's unnormal values".


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:1948
+                                          const APFloat &Src) {
+  // I sure hope zero, positive and negative, is always OK to fold.
+  if (Src.isZero())
----------------
spatel wrote:
> Delete the "I sure hope" - if we're coding it this way, it's the law. If there's fallout, it'll change. :)
If I'm reading the source code of APFloat, and of glibc, correctly, then APFloat considers {0.0, *} to be isZero() for ppc_fp128, which shouldn't be considered a canonical value.

You should be fine for x86_fp80.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:1966
+    if (DenormMode == DenormalMode::getIEEE())
+      return nullptr;
+
----------------
Denormal in IEEE denormal mode should be canonical, right? Or are you using this mode to assume "it's dynamic, so we don't know if it's canonical or not" (in which case, add a comment to this effect)?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137858/new/

https://reviews.llvm.org/D137858