[PATCH] D134725: [ADT] Add support for more formats in APFixedPoint

[Mikael Holmén via Phabricator via llvm-commits]

uabelho added inline comments.


================
Comment at: llvm/include/llvm/ADT/APFixedPoint.h:216-217
+        (getLsbWeight() > 0) ? Val.extend(getWidth() + getLsbWeight()) : Val;
     if (Val < 0 && Val != -Val) // Cover the case when we have the min val
-      return -(-Val >> getScale());
-    else
-      return Val >> getScale();
+      return -(-ExtVal.relativeShl(getLsbWeight()));
+    return ExtVal.relativeShl(getLsbWeight());
----------------
Tyker wrote:
> leonardchan wrote:
> > At a glance, I think the issue might be here. The 16-bit wide APSInt is -3276. Prior to this patch, the order was negate Val (3276), shift (3276 >> 15 == 0), then negate again (0). Now the updated process looks to be shift (-3276 >> 15 == -1), negate (1), then negate again (-1). Looks like the right fix is `-((-ExtVal).relativeShl(getLsbWeight()))`. @Tyker could you confirm this?
> You are right, this is incorrect.
@Tyker : Can you push a fix for this?


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

https://reviews.llvm.org/D134725