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

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added a comment.

In D134725#3842861 <https://reviews.llvm.org/D134725#3842861>, @uabelho wrote:

> In D134725#3842813 <https://reviews.llvm.org/D134725#3842813>, @uabelho wrote:
>
>> In D134725#3842654 <https://reviews.llvm.org/D134725#3842654>, @uabelho wrote:
>>
>>> Hi,
>>> Just a heads-up that we see miscompiles with this patch for our out-of-tree target.
>>> It might very well be something in our code that is broken, just thought I'd mention it in case someone else have problems too.
>>> I'll try to debug our problems and see what's up.
>>
>> I don't have any details about how, but it's a cast from a negative fixed point number to an integer that goes wrong now.
>> So -0.1r is turned into -1 instead of the expected value 0.
>
> I still really don't know how this should work and what's happening but I added some printouts to getIntPart() and got
>
>   getIntPart: APFixedPoint(-0.0999755859375, {width=16, scale=15, msb=0, lsb=-15, IsSigned=1, HasUnsignedPadding=0, IsSaturated=0})getMsbWeight(): 0
>   getLsbWeight(): -15
>   ExtVal: -3276
>   Val: -3276
>   res: -1
>
> So to me it looks like it's converting -0.1 to -1 there, which is wrong i think?

Yeah the default rounding mode should be towards zero. `getIntPart` should also return 0 for `-0.0999755859375`.



================
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());
----------------
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?


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

https://reviews.llvm.org/D134725