[Fixed Point Arithmetic] Rename `-fsame-fbits` flag

[leonardchan]

Summary

• Rename the -fsame-fbits flag to -fpadding-on-unsigned-fixed-point
• Move the flag from a driver option to a cc1 option
• Rename the SameFBits member in TargetInfo to PaddingOnUnsignedFixedPoint
• Updated descriptions

[leonardchan]

Formatting and forgot to get rid of Group<f_Group> and Flags<CC1Option>

[rjmccall]

This all looks reasonable except that I think the interpretation is exactly backwards, no? From the documentation on the option, -fpadding-on-unsigned-fixed-point causes there to be padding, i.e. the inverse of the old SameFBits; but the default value, comments, and boolean checks throughout the code are all still using the old interpretation.

[leonardchan]

Oh, having the same number of fractional bits is what leads to unsigned types having one bit of padding, and vice versa.

If this flag is set to false, then the integral and fractional parts of the unsigned types take up the whole bit width of the underlying scaled integer, with unsigned types gaining one extra fractional bit which would be taken by the sign bit in the signed types. When this flag is true, the number of fractional bits for unsigned types changes to match the signed types, but the number of integral bits stays the same, which leads to one unused padding bit.

[rjmccall]

In D48727#1146873, @leonardchan wrote:

Oh, having the same number of fractional bits is what leads to unsigned types having one bit of padding, and vice versa.

If this flag is set to false, then the integral and fractional parts of the unsigned types take up the whole bit width of the underlying scaled integer, with unsigned types gaining one extra fractional bit which would be taken by the sign bit in the signed types. When this flag is true, the number of fractional bits for unsigned types changes to match the signed types, but the number of integral bits stays the same, which leads to one unused padding bit.

Oh, of course. Sorry for the confusion; LGTM.

include/clang/Basic/LangOptions.def

 
 ENUM_LANGOPT(ClangABICompat, ClangABI, 4, ClangABI::Latest,
              "version of Clang that we should attempt to be ABI-compatible "
              "with")
 
 COMPATIBLE_VALUE_LANGOPT(FunctionAlignment, 5, 0, "Default alignment for functions")
 
 LANGOPT(FixedPoint, 1, 0, "fixed point types")
-LANGOPT(SameFBits, 1, 0,
-        "unsigned and signed fixed point type having the same number of fractional bits")
+LANGOPT(PaddingOnUnsignedFixedPoint, 1, 0,
+        "unsigned fixed point types having one extra padding bit")
 
 #undef LANGOPT
 #undef COMPATIBLE_LANGOPT
 #undef BENIGN_LANGOPT
 #undef ENUM_LANGOPT
 #undef COMPATIBLE_ENUM_LANGOPT
 #undef BENIGN_ENUM_LANGOPT
 #undef VALUE_LANGOPT
 #undef COMPATIBLE_VALUE_LANGOPT
 #undef BENIGN_VALUE_LANGOPT

include/clang/Basic/TargetInfo.h

protected:
   unsigned char ShortAccumWidth, ShortAccumAlign;
   unsigned char AccumWidth, AccumAlign;
   unsigned char LongAccumWidth, LongAccumAlign;
   unsigned char ShortFractWidth, ShortFractAlign;
   unsigned char FractWidth, FractAlign;
   unsigned char LongFractWidth, LongFractAlign;
 
   // If true, unsigned fixed point types have the same number of fractional bits
-  // as their signed counterparts. Otherwise, unsigned fixed point types have
+  // as their signed counterparts, forcing the unsigned types to have one extra
+  // bit of padding. Otherwise, unsigned fixed point types have
   // one more fractional bit than its corresponding signed type. This is false
   // by default.
-  bool SameFBits;
+  bool PaddingOnUnsignedFixedPoint;
 
   // Fixed point integral and fractional bit sizes
   // Saturated types share the same integral/fractional bits as their
   // corresponding unsaturated types.
   // For simplicity, the fractional bits in a _Fract type will be one less the
   // width of that _Fract type. This leaves all signed _Fract types having no
   // padding and unsigned _Fract types will only have 1 bit of padding after the
-  // sign if SameFBits is set.
+  // sign if PaddingOnUnsignedFixedPoint is set.
   unsigned char ShortAccumScale;
   unsigned char AccumScale;
   unsigned char LongAccumScale;
 
   unsigned char SuitableAlign;
   unsigned char DefaultAlignForAttributeAligned;
   unsigned char MinGlobalAlign;
   unsigned char MaxAtomicPromoteWidth, MaxAtomicInlineWidth;
public:
   unsigned getLongAccumScale() const { return LongAccumScale; }
   unsigned getLongAccumIBits() const {
     return LongAccumWidth - LongAccumScale - 1;
   }
 
   /// getUnsignedShortAccumScale/IBits - Return the number of
   /// fractional/integral bits in a 'unsigned short _Accum' type.
   unsigned getUnsignedShortAccumScale() const {
-    return SameFBits ? ShortAccumScale : ShortAccumScale + 1;
+    return PaddingOnUnsignedFixedPoint ? ShortAccumScale : ShortAccumScale + 1;
   }
   unsigned getUnsignedShortAccumIBits() const {
-    return SameFBits ? getShortAccumIBits()
-                     : ShortAccumWidth - getUnsignedShortAccumScale();
+    return PaddingOnUnsignedFixedPoint
+               ? getShortAccumIBits()
   }
 
   /// getUnsignedAccumScale/IBits - Return the number of fractional/integral
   /// bits in a 'unsigned _Accum' type.
   unsigned getUnsignedAccumScale() const {
-    return SameFBits ? AccumScale : AccumScale + 1;
+    return PaddingOnUnsignedFixedPoint ? AccumScale : AccumScale + 1;
   }
   unsigned getUnsignedAccumIBits() const {
-    return SameFBits ? getAccumIBits() : AccumWidth - getUnsignedAccumScale();
+    return PaddingOnUnsignedFixedPoint ? getAccumIBits()
+                                       : AccumWidth - getUnsignedAccumScale();
   }
 
   /// getUnsignedLongAccumScale/IBits - Return the number of fractional/integral
   /// bits in a 'unsigned long _Accum' type.
   unsigned getUnsignedLongAccumScale() const {
-    return SameFBits ? LongAccumScale : LongAccumScale + 1;
+    return PaddingOnUnsignedFixedPoint ? LongAccumScale : LongAccumScale + 1;
   }
   unsigned getUnsignedLongAccumIBits() const {
-    return SameFBits ? getLongAccumIBits()
-                     : LongAccumWidth - getUnsignedLongAccumScale();
+    return PaddingOnUnsignedFixedPoint
+               ? getLongAccumIBits()
   }
 
   /// getShortFractScale - Return the number of fractional bits
   /// in a 'signed short _Fract' type.
   unsigned getShortFractScale() const { return ShortFractWidth - 1; }
 
   /// getFractScale - Return the number of fractional bits
   /// in a 'signed _Fract' type.
   unsigned getFractScale() const { return FractWidth - 1; }
 
   /// getLongFractScale - Return the number of fractional bits
   /// in a 'signed long _Fract' type.
   unsigned getLongFractScale() const { return LongFractWidth - 1; }
 
   /// getUnsignedShortFractScale - Return the number of fractional bits
   /// in a 'unsigned short _Fract' type.
   unsigned getUnsignedShortFractScale() const {
-    return SameFBits ? getShortFractScale() : getShortFractScale() + 1;
+    return PaddingOnUnsignedFixedPoint ? getShortFractScale()
+                                       : getShortFractScale() + 1;
   }
 
   /// getUnsignedFractScale - Return the number of fractional bits
   /// in a 'unsigned _Fract' type.
   unsigned getUnsignedFractScale() const {
-    return SameFBits ? getFractScale() : getFractScale() + 1;
+    return PaddingOnUnsignedFixedPoint ? getFractScale() : getFractScale() + 1;
   }
 
   /// getUnsignedLongFractScale - Return the number of fractional bits
   /// in a 'unsigned long _Fract' type.
   unsigned getUnsignedLongFractScale() const {
-    return SameFBits ? getLongFractScale() : getLongFractScale() + 1;
+    return PaddingOnUnsignedFixedPoint ? getLongFractScale()
+                                       : getLongFractScale() + 1;
   }
 
   /// Determine whether the __int128 type is supported on this target.
   virtual bool hasInt128Type() const {
     return (getPointerWidth(0) >= 64) || getTargetOpts().ForceEnableInt128;
   } // FIXME
 
   /// Determine whether _Float16 is supported on this target.

include/clang/Driver/CC1Options.td

 }
 
 def target_linker_version : Separate<["-"], "target-linker-version">,
   HelpText<"Target linker version">;
 def triple_EQ : Joined<["-"], "triple=">, Alias<triple>;
 def mfpmath : Separate<["-"], "mfpmath">,
   HelpText<"Which unit to use for fp math">;
 
+def fpadding_on_unsigned_fixed_point : Flag<["-"], "fpadding-on-unsigned-fixed-point">,
+      HelpText<"Force each unsigned fixed point type to have an extra bit of padding to align their scales with those of signed fixed point types">;
+def fno_padding_on_unsigned_fixed_point : Flag<["-"], "fno-padding-on-unsigned-fixed-point">;
+
 //===----------------------------------------------------------------------===//
 // Analyzer Options
 //===----------------------------------------------------------------------===//
 
 def analysis_UnoptimizedCFG : Flag<["-"], "unoptimized-cfg">,
   HelpText<"Generate unoptimized CFGs for all analyses">;
 def analysis_CFGAddImplicitDtors : Flag<["-"], "cfg-add-implicit-dtors">,
   HelpText<"Add C++ implicit destructors to CFGs for all analyses">;

include/clang/Driver/Options.td

 def fno_force_enable_int128 : Flag<["-"], "fno-force-enable-int128">,
   Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Disable support for int128_t type">;
 
 def ffixed_point : Flag<["-"], "ffixed-point">, Group<f_Group>,
                    Flags<[CC1Option]>, HelpText<"Enable fixed point types">;
 def fno_fixed_point : Flag<["-"], "fno-fixed-point">, Group<f_Group>,
                       HelpText<"Disable fixed point types">;
-def fsame_fbits : Flag<["-"], "fsame-fbits">, Group<f_Group>,
-                  Flags<[CC1Option]>,
-      HelpText<"Force each unsigned fixed point type to have the same number of fractional bits as its corresponding signed type">;
-def fno_same_fbits : Flag<["-"], "fno-same-fbits">, Group<f_Group>;
 
 // Begin sanitizer flags. These should all be core options exposed in all driver
 // modes.
 let Flags = [CC1Option, CoreOption] in {
 
 def fsanitize_EQ : CommaJoined<["-"], "fsanitize=">, Group<f_clang_Group>,
                    MetaVarName<"<check>">,
                    HelpText<"Turn on runtime checks for various forms of undefined "

lib/Basic/TargetInfo.cpp

TargetInfo::TargetInfo(const llvm::Triple &T) : TargetOpts(), Triple(T) {
   ShortFractWidth = ShortFractAlign = 8;
   FractWidth = FractAlign = 16;
   LongFractWidth = LongFractAlign = 32;
 
   // Fixed point default integral and fractional bit sizes
   // We give the _Accum 1 fewer fractional bits than their corresponding _Fract
   // types by default to have the same number of fractional bits between _Accum
   // and _Fract types.
-  SameFBits = false;
+  PaddingOnUnsignedFixedPoint = false;
   ShortAccumScale = 7;
   AccumScale = 15;
   LongAccumScale = 31;
 
   SuitableAlign = 64;
   DefaultAlignForAttributeAligned = 128;
   MinGlobalAlign = 0;
   // From the glibc documentation, on GNU systems, malloc guarantees 16-byte
if (Opts.OpenCL) {
     LongDoubleFormat = &llvm::APFloat::IEEEquad();
   }
 
   if (Opts.NewAlignOverride)
     NewAlign = Opts.NewAlignOverride * getCharWidth();
 
   // Each unsigned fixed point type has the same number of fractional bits as
   // its corresponding signed type.
-  SameFBits |= Opts.SameFBits;
+  PaddingOnUnsignedFixedPoint |= Opts.PaddingOnUnsignedFixedPoint;
   CheckFixedPointBits();
 }
 
 bool TargetInfo::initFeatureMap(
     llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags, StringRef CPU,
     const std::vector<std::string> &FeatureVec) const {
   for (const auto &F : FeatureVec) {
     StringRef Name = F;

lib/Driver/ToolChains/Clang.cpp

#endif
   Args.AddLastArg(CmdArgs, options::OPT_pedantic_errors);
   Args.AddLastArg(CmdArgs, options::OPT_w);
 
   // Fixed point flags
   if (Args.hasFlag(options::OPT_ffixed_point, options::OPT_fno_fixed_point,
                    /*Default=*/false))
     Args.AddLastArg(CmdArgs, options::OPT_ffixed_point);
 
-  if (Args.hasFlag(options::OPT_fsame_fbits,
-                   options::OPT_fno_same_fbits,
-                   /*Default=*/false))
-    Args.AddLastArg(CmdArgs, options::OPT_fsame_fbits);
-
   // Handle -{std, ansi, trigraphs} -- take the last of -{std, ansi}
   // (-ansi is equivalent to -std=c89 or -std=c++98).
   //
   // If a std is supplied, only add -trigraphs if it follows the
   // option.
   bool ImplyVCPPCXXVer = false;
   if (Arg *Std = Args.getLastArg(options::OPT_std_EQ, options::OPT_ansi)) {
     if (Std->getOption().matches(options::OPT_ansi))

lib/Frontend/CompilerInvocation.cpp

#include "clang/Frontend/LangStandards.def"
   Opts.Exceptions = Args.hasArg(OPT_fexceptions);
   Opts.ObjCExceptions = Args.hasArg(OPT_fobjc_exceptions);
   Opts.CXXExceptions = Args.hasArg(OPT_fcxx_exceptions);
 
   // -ffixed-point
   Opts.FixedPoint =
       Args.hasFlag(OPT_ffixed_point, OPT_fno_fixed_point, /*Default=*/false) &&
       !Opts.CPlusPlus;
-  Opts.SameFBits =
-      Args.hasFlag(OPT_fsame_fbits, OPT_fno_same_fbits,
+  Opts.PaddingOnUnsignedFixedPoint =
+      Args.hasFlag(OPT_fpadding_on_unsigned_fixed_point,
+                   OPT_fno_padding_on_unsigned_fixed_point,
                    /*Default=*/false) &&
       Opts.FixedPoint;
 
   // Handle exception personalities
   Arg *A = Args.getLastArg(options::OPT_fsjlj_exceptions,
                            options::OPT_fseh_exceptions,
                            options::OPT_fdwarf_exceptions);
   if (A) {

lib/Sema/SemaExpr.cpp

if (Literal.isFixedPointLiteral()) {
     unsigned scale = Context.getFixedPointScale(Ty);
     unsigned ibits = Context.getFixedPointIBits(Ty);
     unsigned bit_width = Context.getTypeInfo(Ty).Width;
 
     llvm::APInt Val(bit_width, 0, isSigned);
     bool Overflowed = Literal.GetFixedPointValue(Val, scale);
 
     // Do not use bit_width since some types may have padding like _Fract or
-    // unsigned _Accums if SameFBits is set.
+    // unsigned _Accums if PaddingOnUnsignedFixedPoint is set.
     auto MaxVal = llvm::APInt::getMaxValue(ibits + scale).zextOrSelf(bit_width);
     if (Literal.isFract && Val == MaxVal + 1)
       // Clause 6.4.4 - The value of a constant shall be in the range of
       // representable values for its type, with exception for constants of a
       // fract type with a value of exactly 1; such a constant shall denote
       // the maximal value for the type.
       --Val;
     else if (Val.ugt(MaxVal) || Overflowed)

test/Frontend/fixed_point_same_fbits.c

 // RUN: %clang -ffixed-point -S -emit-llvm -o - %s | FileCheck %s -check-prefix=DEFAULT
-// RUN: %clang -ffixed-point -fsame-fbits -S -emit-llvm -o - %s | FileCheck %s -check-prefix=SAME
+// RUN: %clang_cc1 -ffixed-point -fpadding-on-unsigned-fixed-point -S -emit-llvm -o - %s | FileCheck %s -check-prefix=SAME
 
 /* The scale for unsigned fixed point types should be the same as that of signed
  * fixed point types when -fsame-fbits is enabled. */
 
 void func() {
   unsigned short _Accum u_short_accum = 0.5uhk;
   unsigned _Accum u_accum = 0.5uk;
   unsigned long _Accum u_long_accum = 0.5ulk;