ebevhan added inline comments.
================
Comment at: lib/CodeGen/CGExprScalar.cpp:768
+ if (CGF.getContext().getTargetInfo().unsignedFixedPointTypesHavePadding()
&&
+ Ty->isUnsignedFixedPointType()) {
+ unsigned NumBits = CGF.getContext().getTypeSize(Ty);
----------------
rjmccall wrote:
> ebevhan wrote:
> > rjmccall wrote:
> > > ebevhan wrote:
> > > > 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.
> > > > 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.
> > >
> > > Ah, I see, and now I've found where this is laid out in the spec, as well
> > > as where the spec leaves this representation question open for unsigned
> > > types.
> > >
> > > Is this review debating whether or not to use a padded implementation?
> > > Do we not have any requirement to maintain compatibility with other
> > > compilers? Do other compilers differ on the representation, perhaps by
> > > target, or is there general agreement?
> > >
> > > In the abstract, I think it would be better to fully use all the bits
> > > available, even if it means conversions are non-trivial.
> > We had some discussions about representation in earlier patches. The target
> > specification was a lot more free in the first revisions, but we locked it
> > down to fewer parameters (scale options for accum types, fract scale being
> > width-1, and the unsigned-signed scale flag) to make the implementation
> > simpler.
> >
> > I argued for adding the target option to set the unsigned scale to the same
> > as the signed; Leonard's original design did not have it. The reason is
> > simply that we use that representation downstream in our DSP-C
> > implementation, and things would be harder to maintain for us if the
> > upstream implementation did not support it. The only other Embedded-C
> > implementation I know of is the one in avr-gcc, and I believe it uses all
> > the bits for unsigned.
> >
> > I've given some arguments for why our representation is better from an
> > implementation perspective (conversion is easier, precision ranges are more
> > uniform, hardware operations that operate on signed can be reused for
> > unsigned) in previous patches, but I suspect that using all bits is a
> > reasonable default.
> I'd be happy to read those discussions instead of asking you to summarize
> them if you wouldn't mind digging out a link.
>
> That said, I agree that the right approach for Clang as a project is to allow
> the target to decide. If there were really an arbitrary number of
> implementations, that would be different, but from the spec it seems clear
> that there are exactly two reasonable choices.
I think most of the discussion about representation is spread out in D46915,
probably some here:
* https://reviews.llvm.org/D46915#inline-419405
* https://reviews.llvm.org/D46915#inline-421591
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
