[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

Bevin Hansson via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Jun 1 00:22:44 PDT 2018


ebevhan added inline comments.


================
Comment at: lib/Basic/TargetInfo.cpp:45
+  AccumWidth = AccumAlign = 32;
+  LongAccumWidth = LongAccumAlign = 64;
   SuitableAlign = 64;
----------------
rsmith wrote:
> leonardchan wrote:
> > leonardchan wrote:
> > > ebevhan wrote:
> > > > leonardchan wrote:
> > > > > rsmith wrote:
> > > > > > jfb wrote:
> > > > > > > This seems weird because Targets which don't have these values for the non-Accum versions will have .e.g. `sizeof(short) != sizeof(short _Accum)`. Is there a point in ever having `_Accum` differ in size, width, and alignment from the underlying type? If not, can you set these values after the sub-target has specified its preferences?
> > > > > > I'm uncomfortable about opting all targets into this behavior with these default values; this will result in an ABI break if later a target updates these to the correct values. A per-target `AccumSupported` flag would help substantially. But this is OK for the short term while you're still working on the feature.
> > > > > > 
> > > > > > We'll also need the target to inform us of the number of integer and fractional bits for each such type.
> > > > > The integral and fractional bits will be set in the target and is available in a later patch.
> > > > > We'll also need the target to inform us of the number of integer and fractional bits for each such type.
> > > > 
> > > > I believe the only one that is needed is for the number of fractional bits; the number of integer bits is implied by the difference between the type width and fractional bits. I think I mention this in one of the other patches.
> > > > 
> > > > 
> > > You're right. I was stuck in the mindset that we would be providing an integral and fractional value.
> > > The integral and fractional bits will be set in the target and is available in a later patch.
> > 
> > I mean just the fractional bits since the integral will just be the bit width minus fractional.
> You're assuming that all bits will be either integral or fractional bits (and in particular that there are no padding bits). I don't know if that's actually going to be true for all target ABIs, but I suppose it's OK to make this assumption until it's proven wrong by some actual target.
It is actually the case for us (downstream) that the unsigned types should have one bit of padding, however, this is a special case. The spec says "Each unsigned fract type has either the same number of fractional bits as, or one more fractional bit than, its corresponding signed fract type." and also under 'recommended practice', "Each signed accum type has the same number of fractional bits as either its corresponding signed fract type or its corresponding unsigned fract type."

So I think the approach would be that the integral bits are implied from the fractional bits and the width, except in the unsigned case where the MSB is a padding bit. If there is a target that really does want the unsigned types to have an extra bit of precision it could be added as a flag, but I don't really see the benefit of it. The consistency benefit of having the unsigned types have the same scaling factor as the signed ones outweighs the downsides.


Repository:
  rC Clang

https://reviews.llvm.org/D46084





More information about the cfe-commits mailing list