[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:126
+  /// \p Ty, or a floating point type with a larger exponent than Ty.
+  Type *getAccommodatingFloatType(Type *Ty, const FixedPointSemantics &Sema) {
+    const fltSemantics *FloatSema = &Ty->getFltSemantics();
----------------
rjmccall wrote:
> I like this method name.  Does this have the same problem as I asked about in the other patch about really needing to be about whether the *unscaled* fixed-point type fits in the given type?
It should be the same case here, yes. Both the integral range and the scale matter when determining this.


================
Comment at: llvm/include/llvm/IR/FixedPointBuilder.h:131
+    // There's seemingly no way to convert fltSemantics to Type.
+    return ConstantFP::get(Ty->getContext(), APFloat(*FloatSema))->getType();
+  }
----------------
rjmccall wrote:
> Could you just extract that code out of `llvm::ConstantFP::get` and put it on `llvm::Type`?  Might be better as a separate patch.
> 
> While you're at it, there's also a `TypeToFloatSemantics` function in Constants.cpp that's completely redundant with `llvm::Type::getFltSemantics`.
Done in D87512.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86632