[PATCH] D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types
Bevin Hansson via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 25 05:07:03 PDT 2018
ebevhan added a comment.
Would it be possible to add some form of target hook (perhaps to CodeGenABIInfo, which is already accessed with `getTargetHooks`) for fixed-point operations (maybe even some conversions)? As I've mentioned earlier, we emit both IR and intrinsics for many of these operations to make instruction selection possible. I suspect that any target that wants to utilize fixed-point effectively will need this as well.
---
I'm contemplating whether the unsigned padding bit clear before operations is needed at all. If you have the outlook that producing a value with a bit in that position is technically overflow and therefore undefined behavior, it shouldn't matter that the bit is set or not before operations; we already have UB. If you want to be sure that things 'work anyway', then you're better off clearing after operations instead to keep things consistent between operations instead of just before them.
I suspect that some of our tests depend on unsigned behaving this way, so we would have to look at that in case we decide to drop the clear altogether...
---
I also have a bit more feedback on previous patches that I missed at the time, but it feels wrong to add that here. How do you want me to take it?
================
Comment at: include/clang/AST/ASTContext.h:1954
+ llvm::APInt getFixedPointMin(QualType Ty) const;
+ llvm::APInt getFixedPointOne(QualType Ty) const;
----------------
rjmccall wrote:
> 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.
All of the parameters that determine these values (min, max, one) need to be sourced from the target, though. It's not like APSInt is aware of the dimensions of integers on the targets. I think these should return APSInt and not APInt, though.
Creating another abstraction on top of APInt is probably still quite useful, especially for certain operations (multiplication, division, saturating conversion, saturating operations). Probably not too useful for this patch since it doesn't deal with constant evaluation.
================
Comment at: include/clang/Lex/LiteralSupport.h:76
+ bool isFixedPointLiteral() const {
+ return saw_fixed_point_suffix && (saw_period || saw_exponent);
+ }
----------------
I realized there was something off with this as well after the patch landed, but I wasn't sure what it was.
Could this cause problems with user-defined literals as well?
================
Comment at: lib/AST/ASTContext.cpp:10260
+
+ if (Ty->isSignedFixedPointType())
+ Val = llvm::APInt::getSignedMaxValue(NumBits);
----------------
Can't this be `Ty->isSignedFixedPointType() || SameFBits`?
I suspect this function can be simplified to look like the 'Min' one.
================
Comment at: lib/AST/ASTContext.cpp:10283
+ llvm::APInt One(NumBits, 1, Ty->isSignedFixedPointType());
+ return One.shl(Scale);
+}
----------------
This will give a nonsensical result for unsigned fract with !SameFBits.
================
Comment at: lib/AST/ExprConstant.cpp:9501
+ return false;
+ return Success(Result.getInt() >> Scale, E);
+ }
----------------
The shift here will not produce the correct rounding behavior for fixed-point to integer conversion. E-C says `Conversions from a fixed-point to an integer type round toward zero.` However, simply shifting will always round towards -inf.
If the fixed-point number is negative, you need to add `lowBits(Scale)` before performing the shift.
This operation here is also not the same as the FixedPointToBool emission in CodeGen.
================
Comment at: lib/CodeGen/CGExprScalar.cpp:768
+ if (CGF.getContext().getTargetInfo().unsignedFixedPointTypesHavePadding() &&
+ Ty->isUnsignedFixedPointType()) {
+ unsigned NumBits = CGF.getContext().getTypeSize(Ty);
----------------
rjmccall wrote:
> 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?
I'll give an example with a fract type. Let's say fract is 16 bits wide. That means the signed fract will have a scale of 15 (15 fractional bits) and one sign bit, for a value range of [-1.0, 1.0). The LSB is worth 2^-15 and the MSB is worth -2^0, similar to signed integers.
The unsigned fract cannot be negative, but must also have a value range of 1.0. This means that either:
* The scale remains the same (15), and the MSB is a padding bit. This bit would normally be the 2^0 bit, but since the range of the number cannot exceed 1, this bit will always be 0. In this case, the LSB is worth 2^-15 (same as the signed) and the MSB is not worth anything.
* The scale is that of signed fract + 1 (16). All bits of the number are value bits, and there is no padding. The LSB is worth 2^-16 and the MSB is worth 2^-1. There is no bit with a magnitude of 2^0.
These rules also apply to accum types, since the number of integral bits in the unsigned accum types may not exceed that of the signed ones. Therefore, we either have to choose between having the same number of fractional bits as the signed ones (leaving the MSB as 0/padding) or having one more fractional bit than the signed ones.
The lowBits is just there to construct a mask to block out the MSB. There should be no endian consideration.
================
Comment at: lib/CodeGen/CGExprScalar.cpp:1790
+ case CK_FixedPointToIntegral: {
+ const QualType &SrcTy = E->getType();
----------------
Is there a reason these are all implemented here and not in EmitScalarConversion?
================
Comment at: lib/CodeGen/CGExprScalar.cpp:1796
+ llvm::Value *Val = ClearFixedPointPadding(SrcTy, Visit(E));
+ if (DestTy->isSignedIntegerType())
+ return Builder.CreateAShr(
----------------
As I mentioned in the ExprConstant case above, the shifting here is not enough to guarantee the correct rounding.
Also, I'm not sure I'm comfortable with relying on EmitScalarConversion for parts of these conversions. If you know what needs to be emitted (addition if negative, right shift, then extension) simply do that explicitly instead of trusting that EmitScalarConversion will do the right thing for you.
For the record, we do the shifting before the extension.
================
Comment at: lib/CodeGen/CGExprScalar.cpp:1801
+ return Builder.CreateLShr(
+ EmitScalarConversion(Val, SrcTy, DestTy, CE->getExprLoc()), scale);
+ }
----------------
rjmccall wrote:
> 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?
I think that EmitScalarConversion will do 'the right thing' since it's being passed an input type of width N and converting it to a type of width M, but it's doing so 'stupidly' and isn't really aware of that one of them is a fixed-point type.
I'd much rather see the conversions be completely separate from EmitScalarConversion, implemented with explicit IR operations. Or have them moved to EmitScalarConversion completely. We've done the latter.
================
Comment at: lib/CodeGen/CGExprScalar.cpp:1815
+ if (DestTy->isSaturatedFixedPointType()) {
+ // Cast both sides up to one more than the highest width of the 2 to
+ // prevent truncating any bits and be able to do a simple signed
----------------
I'm not sure I understand what 'both sides' are here. You should not have to compare the casted source integer at all; it should be possible to determine saturation based solely on the original value.
I think there are three cases (disregarding sign, which I know plays a part but I'm unsure at first glance how to handle):
* src has fewer integral bits than dst has ibits; saturation cannot occur.
* src has the same number of integral bits as dst has ibits; I believe it comes down to signedness here.
* src has more integral bits than dst has ibits; this is where you need a comparison.
Ultimately it boils down to determining if the source integer is outside the value range of the integral portion of the fixed-point value, which should not require any casting of the integer other than to produce the value normally (as in `Result` above).
================
Comment at: lib/CodeGen/CGExprScalar.cpp:1882
+
+ llvm::APInt Normalizer = CGF.getContext().getFixedPointOne(DestTy);
+ llvm::APFloat NormalizerFlt(SrcSema, 1);
----------------
Won't this be zero for unsigned fract if SameFBits is false?
================
Comment at: lib/CodeGen/CGExprScalar.cpp:1949
+ bool isSigned = SrcTy->isSignedFixedPointType();
+
+ if (DstWidth > SrcWidth)
----------------
In our downstream, we interleave the rescaling and resizing differently. Grabbed from our consteval:
```
if (DestScale < SrcScale)
Result = Result >> (SrcScale - DestScale);
Result = Result.extOrTrunc(DestWidth);
Result.setIsUnsigned(DestType->isUnsignedFixedPointType());
if (DestScale > SrcScale)
Result = Result << (DestScale - SrcScale);
```
I'm not sure if this difference matters from a functional perspective at first glance. Something like:
```
if (DstScale < SrcScale)
Result = isSigned ? Builder.CreateAShr(Result, SrcScale - DstScale)
: Builder.CreateLShr(Result, SrcScale - DstScale);
if (DstWidth != SrcWidth)
// Resize
Result = Builder.CreateIntCast(
Result, CGF.CGM.getTypes().ConvertTypeForMem(DestTy), isSigned);
if (DstScale > SrcScale) {
Result = Builder.CreateShl(Result, DstScale - SrcScale);
}
```
Also, the code here doesn't seem to handle saturation. That can get a bit tricky when casting between types of differing scale.
================
Comment at: lib/Sema/SemaCast.cpp:2612
+ // Check on fixed point types
+ if (SrcType->isFixedPointType() &&
+ !Sema::CheckSupportedFixedPointCast(DestType)) {
----------------
Why are these checks needed? Won't `PrepareScalarCast` guard against invalid fixed-point casts?
================
Comment at: lib/Sema/SemaExpr.cpp:6904
+ // Check on fixed point types
+ if (RHSTy->isFixedPointType() && !Sema::CheckSupportedFixedPointCast(LHSTy)) {
----------------
I'm not sure why this is needed. All conversions between fixed-point types and other arithmetic types are defined, at least in Embedded-C (not in DSP-C, so we require a patch here).
Also, this is for the conditional operator, so getting an error about 'casts' seems like it could be very confusing. Is there anything that's preventing you from getting the `err_typecheck_cond_incompatible_operands` error at the very bottom?
================
Comment at: lib/Sema/SemaExpr.cpp:7761
+ // Check on fixed point types
+ if ((RHSType->isFixedPointType() &&
----------------
Same as the other case; why are these checks needed?
Repository:
rC Clang
https://reviews.llvm.org/D48456
More information about the cfe-commits
mailing list