[PATCH] D86632: [Fixed Point] Add codegen for conversion between fixed-point and floating point.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.


================
Comment at: llvm/include/llvm/IR/FixedPointBuilder.h:171-172
+    // lossless, except for overflow to infinity which is unlikely.
+    return B.CreateFMul(Result,
+        ConstantFP::get(DstTy, std::pow(2, -(int)SrcSema.getScale())));
+  }
----------------
leonardchan wrote:
> Was there a reason for preferring multiplying by the reciprocal instead of dividing by a power of 2?
> 
> I think this was discussed before, but can't seem to remember/find the conversation.
Multiplication should be more efficient than division with the same effect.

However, as @rjmccall mentioned in D54749, this rescaling might not work if the exponent is not large enough to fit the scaled value. This is unlikely for single precision float and larger formats, but it probably won't work for half precision.

I'm not entirely sure what to do about that. I was originally planning on adding an intrinsic that did pretty much the same thing as the one in that patch, but with a scaling factor. However, I figured that it would not be necessary when I noticed that an integer intrinsic was already in the works.

It feels odd to have intrinsics that do pretty much the same thing as another intrinsic but with an internal multiplication baked in. On top of that, it would require adding 4 new intrinsics (signed/unsigned, nonsaturating/saturating) which seems like a bit much.

Perhaps it may be necessary to emit something else when half precision is involved here...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86632