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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
