[PATCH] D149590: ValueTracking: Implement computeKnownFPClass for ldexp

[Matt Arsenault via Phabricator via llvm-commits]

arsenm marked 5 inline comments as done.
arsenm added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4829-4830
+
+        // No-zero can be preserved if there's no potential for a denormal
+        // flush.
+        if (KnownSrc.isKnownNever(fcNegative))
----------------
foad wrote:
> Typo for "non-zero"? But in any case I'm not sure what the comment means or how it relates to this part of the code. Non-zeroness of the input won't be preserved if exp is negative - a non-zero input could easily underflow to a zero result.
The point was this is a slightly stronger version of the previous block checking cannotBeOrdered{Less|Greater}ThanZero that covers 0. The sign isn't changing regardless of overflow 


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4840
+          break;
+        if ((KnownSrc.KnownFPClasses & ExpInfoMask) == fcNone)
+          break;
----------------
foad wrote:
> 
This regresses a few cases. The independent +/- pieces can be inferred below 


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4849
+        if (ExpKnownBits.isZero()) {
+          // ldexp(x, 0) -> x, so propagate everything.
+          Known.propagateCanonicalizingSrc(KnownSrc, *II->getFunction(),
----------------
foad wrote:
> This should have been simplified already to an explicit canonicalize, right?
No, non-constrained operations aren't guaranteed to canonicalize. Nothing folds to canonicalize. This should have folded to a direct x reference. We aren't reporting that the result is canonical, it's reporting that it could have been flushed.

The simplification for this also checks for a literal 0, this query is the full known class query. Theoretically a case could exist that's known 0 but isn't a constant 0


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

https://reviews.llvm.org/D149590