[PATCH] D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/AST/ASTContext.h:1954
+  llvm::APInt getFixedPointMin(QualType Ty) const;
+  llvm::APInt getFixedPointOne(QualType Ty) const;
 
----------------
Are these opaque bit-patterns?  I think there should be a type which abstracts over constant fixed-point values, something `APSInt`-like that also carries the signedness and scale.  For now that type can live in Clang; if LLVM wants to add intrinsic support for fixed-point, it'll be easy enough to move it there.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:768
+    if (CGF.getContext().getTargetInfo().unsignedFixedPointTypesHavePadding() &&
+        Ty->isUnsignedFixedPointType()) {
+      unsigned NumBits = CGF.getContext().getTypeSize(Ty);
----------------
Can you explain the padding thing?  Why is padding uniformly present or absent on all unsigned fixed point types on a target, and never on signed types?  Is this "low bits set" thing endian-specific?


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1801
+      return Builder.CreateLShr(
+          EmitScalarConversion(Val, SrcTy, DestTy, CE->getExprLoc()), scale);
+  }
----------------
Please abstract functions for doing this kind of arithmetic instead of inlining them into scalar emission.  Feel free to make a new CGFixedPoint.h header and corresponding implementation file.

Also, it looks like you're assuming that the output type is the same size as the input type.  I don't think that's actually a language restriction, right?


Repository:
  rC Clang

https://reviews.llvm.org/D48456