Decimal float impl #1

shafik · 2023-11-01T04:56:59Z

This is basis for some discussions about APDecimalFloat, APDecimalFloatLiteral, APDecimalFloatStorage and DecimalFloatExprEvaluator as well as support classes.

You will see that I made the type as integer type because during codegen I could not get it to work any other way. I spent a while trying to disentangle a way to make it work otherwise without success. Floating point type seems to be deeply embedded in the codegen and taking the same path FixedPoint took felt like a much easier row.

There is a lot of hacky code to get the intel decimal float library integrated and you can see in some of the reading of decimal float literals and math I am using the library. You can see that underneath the type used for BID are all integral types.

I could compile code like this:

#include <stdio.h>

void f(_Decimal32 x ) {

}

int main() {
  const _Decimal32 x = 1.0df;
  const _Decimal32 y = 2.0df;
  const _Decimal32 z = x;

  1.0df;

  //static const int ix = 1 + 2;
  static const _Decimal32 a = 1.0df + 2.0df;

  //f(x);

  //x && y;

  //x * y;
  printf("Add\n");
  x - y;

  printf("Sub\n");
  x + y;

  _Decimal32 x2 = 1.0df;
  _Decimal32 y2 = 2.0df;

}

and dump the ast:

-FunctionDecl 0x12e9361b0 <line:7:1, line:31:1> line:7:5 main 'int ()'
  `-CompoundStmt 0x12e936b20 <col:12, line:31:1>
    |-DeclStmt 0x12e936300 <line:8:3, col:29>
    | `-VarDecl 0x12e936268 <col:3, col:24> col:20 used x 'const _Decimal32' cinit
    |   `-DecimalFloatLiteral 0x12e9362d0 <col:24> '_Decimal32' +10E-1
    |-DeclStmt 0x12e9363c0 <line:9:3, col:29>
    | `-VarDecl 0x12e936328 <col:3, col:24> col:20 used y 'const _Decimal32' cinit
    |   `-DecimalFloatLiteral 0x12e936390 <col:24> '_Decimal32' +20E-1
    |-DeclStmt 0x12e936488 <line:10:3, col:25>
    | `-VarDecl 0x12e9363e8 <col:3, col:24> col:20 z 'const _Decimal32' cinit
    |   `-ImplicitCastExpr 0x12e936470 <col:24> '_Decimal32' <LValueToRValue>
    |     `-DeclRefExpr 0x12e936450 <col:24> 'const _Decimal32' lvalue Var 0x12e936268 'x' 'const _Decimal32'
    |-DecimalFloatLiteral 0x12e9364a0 <line:12:3> '_Decimal32' +10E-1
    |-DeclStmt 0x12e9365c8 <line:15:3, col:44>
    | `-VarDecl 0x12e9364e0 <col:3, col:39> col:27 a 'const _Decimal32' static cinit
    |   `-BinaryOperator 0x12e9365a8 <col:31, col:39> '_Decimal32' '+'
    |     |-DecimalFloatLiteral 0x12e936548 <col:31> '_Decimal32' +10E-1
    |     `-DecimalFloatLiteral 0x12e936578 <col:39> '_Decimal32' +20E-1
    |-CallExpr 0x12e9366b0 <line:22:3, col:17> 'int'
    | |-ImplicitCastExpr 0x12e936698 <col:3> 'int (*)(const char *, ...)' <FunctionToPointerDecay>
    | | `-DeclRefExpr 0x12e9365e0 <col:3> 'int (const char *, ...)' Function 0x12e92dcd8 'printf' 'int (const char *, ...)'
    | `-ImplicitCastExpr 0x12e9366f0 <col:10> 'const char *' <NoOp>
    |   `-ImplicitCastExpr 0x12e9366d8 <col:10> 'char *' <ArrayToPointerDecay>
    |     `-StringLiteral 0x12e936638 <col:10> 'char[5]' lvalue "Add\n"
    |-BinaryOperator 0x12e936778 <line:23:3, col:7> '_Decimal32' '-'
    | |-ImplicitCastExpr 0x12e936748 <col:3> '_Decimal32' <LValueToRValue>
    | | `-DeclRefExpr 0x12e936708 <col:3> 'const _Decimal32' lvalue Var 0x12e936268 'x' 'const _Decimal32'
    | `-ImplicitCastExpr 0x12e936760 <col:7> '_Decimal32' <LValueToRValue>
    |   `-DeclRefExpr 0x12e936728 <col:7> 'const _Decimal32' lvalue Var 0x12e936328 'y' 'const _Decimal32'
    |-CallExpr 0x12e9368b8 <line:25:3, col:17> 'int'
    | |-ImplicitCastExpr 0x12e9368a0 <col:3> 'int (*)(const char *, ...)' <FunctionToPointerDecay>
    | | `-DeclRefExpr 0x12e936848 <col:3> 'int (const char *, ...)' Function 0x12e92dcd8 'printf' 'int (const char *, ...)'
    | `-ImplicitCastExpr 0x12e9368f8 <col:10> 'const char *' <NoOp>
    |   `-ImplicitCastExpr 0x12e9368e0 <col:10> 'char *' <ArrayToPointerDecay>
    |     `-StringLiteral 0x12e936868 <col:10> 'char[5]' lvalue "Sub\n"
    |-BinaryOperator 0x12e936980 <line:26:3, col:7> '_Decimal32' '+'
    | |-ImplicitCastExpr 0x12e936950 <col:3> '_Decimal32' <LValueToRValue>
    | | `-DeclRefExpr 0x12e936910 <col:3> 'const _Decimal32' lvalue Var 0x12e936268 'x' 'const _Decimal32'
    | `-ImplicitCastExpr 0x12e936968 <col:7> '_Decimal32' <LValueToRValue>
    |   `-DeclRefExpr 0x12e936930 <col:7> 'const _Decimal32' lvalue Var 0x12e936328 'y' 'const _Decimal32'
    |-DeclStmt 0x12e936a48 <line:28:3, col:24>
    | `-VarDecl 0x12e9369b0 <col:3, col:19> col:14 x2 '_Decimal32' cinit
    |   `-DecimalFloatLiteral 0x12e936a18 <col:19> '_Decimal32' +10E-1
    `-DeclStmt 0x12e936b08 <line:29:3, col:24>
      `-VarDecl 0x12e936a70 <col:3, col:19> col:14 y2 '_Decimal32' cinit
        `-DecimalFloatLiteral 0x12e936ad8 <col:19> '_Decimal32' +20E-1

… us to lex and parse. This include stubbing out APDecimalFloat to represent decimal float in the AST and for calculations. This also includes decFltSemantics which we need to represent the semantics of decimal float types.

…oat constants and math working

zahiraam · 2023-11-01T20:08:20Z

clang/CMakeLists.txt


 set(CLANG_INSTALL_LIBDIR_BASENAME "lib${CLANG_LIBDIR_SUFFIX}")

+


unrelated change. The one below too.

zahiraam · 2023-11-01T20:09:33Z

clang/include/clang/AST/Expr.h

+public:
+  // llvm::APDecimalFloat
+  llvm::APInt getValue(const llvm::decFltSemantics &Semantics) const {
+    // return llvm::APDecimalFloat(getIntValue(),Semantics);


No need for this anymore, right? Same below.

zahiraam · 2023-11-01T20:11:00Z

clang/include/clang/AST/Expr.h

+
+ public:
+  DecimalFloatLiteral(const ASTContext &C, const llvm::APDecimalFloat &V, QualType type,
+                    SourceLocation l, unsigned width = 32 );


Do we need to have width to be default?

zahiraam · 2023-11-09T13:49:55Z

clang/include/clang/AST/Expr.h

+
+  void setWidth(unsigned Width) { width = Width;}
+  unsigned getWidth() { return width; }
+


We will need here get/set scale too.

zahiraam · 2023-11-09T21:04:04Z

I made a first pass through the patch, mostly looking at APDecimalFloat and decFltSemantics and this seems to be moving in the right direction. I think using the integer type is the way to go. This is also the way ICC implements it.

For the arithmetic operations I suppose we would need to have the 64 and 128 flavors for them, right?

tahonermann

I got about half way through the changes today. Overall, the direction looks good to me; I'm just finding more nit-picky detail concerns.

tahonermann · 2023-11-14T21:30:35Z

clang/include/clang/AST/ASTContext.h

+  CanQualType DecimalFloat32Ty, DecimalFloat64Ty, DecimalFloat128Ty; // ISO/IEC TS 18661-2:2015 C23 conditionally supported
+                                                                     // ISO/IEC TR 24733:2011 C++ support


Per https://llvm.org/docs/CodingStandards.html#source-code-width, limit line length to 80 characters.

tahonermann · 2023-11-14T21:42:24Z

clang/include/clang/AST/OperationKinds.def

 CAST_OPERATION(IntegralToFixedPoint)

-/// CK_FixedPointToBoolean - Fixed point to boolean.
+/// CK_FixedPointToBoolean - Decimal Float to boolean.


The change to this comment doesn't seem right.

tahonermann · 2023-11-14T21:49:01Z

clang/include/clang/AST/OperationKinds.def

+/// CK_FloatingToDecimalFloat - Floating to Decimal Float.
+///    _Accum a = f;
+CAST_OPERATION(FloatingToDecimalFloat)
+
+/// CK_DecimalFloatToFloating - Decimal Float to floating.
+///    (float) 2.5k
+CAST_OPERATION(DecimalFloatToFloating)
+
+/// CK_DecimalFloatCast - Decimal Float to Decimal Float.
+///    (_Accum) 0.5r
+CAST_OPERATION(DecimalFloatCast)
+
+/// CK_DecimalFloatToIntegral - Decimal Float to integral.
+///    (int) 2.0k
+CAST_OPERATION(DecimalFloatToIntegral)
+
+/// CK_IntegralToDecimalFloat - Integral to a Decimal Float.
+///    (_Accum) 2
+CAST_OPERATION(IntegralToDecimalFloat)
+
+/// CK_DecimalFloatToBoolean - Decimal Float to boolean.
+///    (bool) 0.5r
+CAST_OPERATION(DecimalFloatToBoolean)


The example cast operations in the comments are still for fixed point types and should be switched to operations on DFP types and literals.

tahonermann · 2023-11-14T21:52:48Z

clang/include/clang/AST/Type.h

  /// Return true if this is a fixed point or integer type.
  bool isFixedPointOrIntegerType() const;

+  /// Return true if this is a decinal float or integer type.


Suggested change

/// Return true if this is a decinal float or integer type.

/// Return true if this is a decimal float or integer type.

tahonermann · 2023-11-14T22:02:33Z

clang/lib/AST/ExprConstant.cpp

+    APSInt AsInt(Val.bitcastToAPInt());
+    return visitInt(AsInt, Ty, Offset);


Do we need to worry about the availability of 128-bit integer support here? I don't think that concern exists for fixed-point types.

tahonermann · 2023-11-14T22:07:35Z

clang/lib/AST/ExprConstant.cpp

+  if (OpOverflow || ConversionOverflow) {
+    if (Info.checkingForUndefinedBehavior())
+      Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
+                                       diag::warn_fixedpoint_constant_overflow)


The referenced diagnostic is for fixed-point, so we presumably need to use the right one (if one for DFP already exists), introduce a new one or, if the existing one is sufficiently generic, rename it. Additional occurrences can be found below.

tahonermann · 2023-11-14T22:38:55Z

clang/lib/AST/ItaniumMangle.cpp

    break;

+  case Expr::DecimalFloatLiteralClass:
+    mangleType(E->getType());


The literal value will need to be mangled here as well; not just the type. See https://godbolt.org/z/Yd8a9ofGn for an example (and a chance to chuckle at the icc behavior).

tahonermann · 2023-11-14T22:42:13Z

clang/lib/AST/ItaniumMangle.cpp

    break;

+  case APValue::DecimalFloat:
+    mangleType(T);


Mangling of the value will be necessary here as well. See https://godbolt.org/z/3Edvh7xa4.

tahonermann · 2023-11-14T22:49:39Z

clang/lib/AST/Type.cpp

  // literal types.
  if (BaseTy->isScalarType() || BaseTy->isVectorType() ||
-      BaseTy->isAnyComplexType())
+      BaseTy->isAnyComplexType() || BaseTy->isDecimalFloatType())


I think DFP types should be considered scalar types; the check for isScalarType() should therefore suffice here.

(note for myself: this is as far as I got with review before running out of time for the day)

tahonermann

I completed my first pass through all of the changes.

I need to spend some time looking at APFloat before weighing in on whether we should try using it first before concluding that an APDecimalFloat is needed. Likewise for fltSemantics vs decFltSemantics.

If you haven't already, can you think about how we can break this up into smaller, but still logically contained, reviews?

tahonermann · 2023-11-16T21:25:15Z

clang/lib/Basic/TargetInfo.cpp

+	DecimalFloat64Width = 64;
+  DecimalFloat64Align = 64;
+	DecimalFloat128Width = 128;


Suggested change

DecimalFloat64Width = 64;

DecimalFloat64Align = 64;

DecimalFloat128Width = 128;

DecimalFloat64Width = 64;

DecimalFloat64Align = 64;

DecimalFloat128Width = 128;

Indentation is off.

tahonermann · 2023-11-16T21:38:48Z

clang/lib/CodeGen/CGDebugInfo.cpp

+  case BuiltinType::DecimalFloat32:
+  case BuiltinType::DecimalFloat64:
+  case BuiltinType::DecimalFloat128:
+    Encoding = llvm::dwarf::DW_ATE_unsigned;


Suggested change

Encoding = llvm::dwarf::DW_ATE_unsigned;

Encoding = llvm::dwarf::DW_ATE_decimal_float;

The DWARF specification already defines an encoding value for the IEEE 754R DFP types and an identifier is present in llvm/include/llvm/BinaryFormat/Dwarf.def, so we can set the right encoding now. I'm not sure if differentiation is required for BID vs DPD encoding here, but we can revisit that as part of tahonermann#20 later.

tahonermann · 2023-11-16T21:50:55Z

clang/lib/CodeGen/CGExprScalar.cpp

+    if (const auto *BinOp = dyn_cast<BinaryOperator>(E)) {
+      QualType LHSType = BinOp->getLHS()->getType();
+      QualType RHSType = BinOp->getRHS()->getType();
+      return LHSType->isDecimalFloatType() || RHSType->isDecimalFloatType();
+    }
+    if (const auto *UnOp = dyn_cast<UnaryOperator>(E))
+      return UnOp->getSubExpr()->getType()->isDecimalFloatType();


I think we need to use getAs() instead of dyn_cast() to look through sugared types like TypedefType. Or do I have that backwards?

tahonermann · 2023-11-16T22:06:04Z

clang/lib/CodeGen/CGExprScalar.cpp

  }
+
+  Value *VisitDecimalFloatLiteral(const DecimalFloatLiteral *E) {
+    return Builder.getInt(E->getValue());


If I'm reading the code right, this will return a llvm::ConstantInt and thereby lose track of the fact that the value corresponds to a DFP representation. Do we not need to keep track of that somehow so that the constant value gets associated with a DFP IR type? That seems important if we're going to utilize the fadd IR instruction/operator.

tahonermann · 2023-11-16T22:07:26Z

clang/lib/CodeGen/CGExprScalar.cpp

+  // Convert to the result type.
+  return FPBuilder.CreateFixedToFixed(
+      Result, IsShift ? LHSFixedSema : CommonFixedSema, ResultFixedSema);
+}


s/Fixed/Decimal/ throughout.

tahonermann · 2023-11-16T22:20:41Z

clang/lib/Sema/SemaExpr.cpp

+    llvm::APDecimalFloat Val(llvm::decFltSemantics(
+        32u, /*Scale=*/1u, /*IsSigned=*/0u, /*IsSaturated=*/false,
+        /*HasUnsignedPadding=*/false));


This will presumably need to be generalized for 64-bit and 128-bit literal support.

tahonermann · 2023-11-16T22:21:43Z

clang/lib/Sema/SemaType.cpp

+    // S.Diag(DS.getTypeSpecTypeLoc(), diag::err_decimal_unsupported);
+    // Result = Context.IntTy;
+    // declarator.setInvalidType(true);


Suggested change

// S.Diag(DS.getTypeSpecTypeLoc(), diag::err_decimal_unsupported);

// Result = Context.IntTy;

// declarator.setInvalidType(true);

Remove.

tahonermann · 2023-11-16T22:28:13Z

llvm/include/llvm/IR/Type.h

    if (getTypeID() == IntegerTyID || isFloatingPointTy() ||
        getTypeID() == PointerTyID || getTypeID() == X86_MMXTyID ||
-        getTypeID() == X86_AMXTyID)
+        getTypeID() == X86_AMXTyID || getTypeID() == DecimalFloat32TyID || getTypeID() == DecimalFloat64TyID || getTypeID() == DecimalFloat128TyID)


Zahira added a isDecimalFloatingPointTy() that can be used here.

tahonermann · 2023-11-16T22:29:21Z

llvm/lib/CodeGen/ValueTypes.cpp

+  case Type::DecimalFloat32TyID:
+    return getIntegerVT(32);


This will presumably need to be generalized for 64-bit and 128-bit support.

tahonermann · 2023-11-16T22:32:11Z

llvm/include/llvm/ADT/APDecimalFloat.h

+  decFltSemantics(unsigned Width, unsigned Scale, bool IsSigned,
+                  bool IsSaturated, bool HasUnsignedPadding)
+      : Width(Width), LsbWeight(-static_cast<int>(Scale)), IsSigned(IsSigned),
+        IsSaturated(IsSaturated), HasUnsignedPadding(HasUnsignedPadding) {}


I think we can omit the saturation properties; we don't need to worry about saturating and non-saturating types like fixed-point does.

Shafik Yaghmour and others added 2 commits October 11, 2022 15:49

Integrating the Intel DFP library as well as getting basic decimal fl…

e9c2ab0

…oat constants and math working

shafik requested review from tahonermann and zahiraam November 1, 2023 15:21

zahiraam reviewed Nov 1, 2023

View reviewed changes

clang/CMakeLists.txt

set(CLANG_INSTALL_LIBDIR_BASENAME "lib${CLANG_LIBDIR_SUFFIX}")

Copy link

Collaborator

zahiraam Nov 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated change. The one below too.

zahiraam reviewed Nov 1, 2023

View reviewed changes

zahiraam reviewed Nov 9, 2023

View reviewed changes

tahonermann reviewed Nov 14, 2023

View reviewed changes

tahonermann reviewed Nov 16, 2023

View reviewed changes


		set(CLANG_INSTALL_LIBDIR_BASENAME "lib${CLANG_LIBDIR_SUFFIX}")


		void setWidth(unsigned Width) { width = Width;}
		unsigned getWidth() { return width; }

		CanQualType DecimalFloat32Ty, DecimalFloat64Ty, DecimalFloat128Ty; // ISO/IEC TS 18661-2:2015 C23 conditionally supported
		// ISO/IEC TR 24733:2011 C++ support

	/// Return true if this is a decinal float or integer type.
	/// Return true if this is a decimal float or integer type.

		APSInt AsInt(Val.bitcastToAPInt());
		return visitInt(AsInt, Ty, Offset);

	Encoding = llvm::dwarf::DW_ATE_unsigned;
	Encoding = llvm::dwarf::DW_ATE_decimal_float;

	// S.Diag(DS.getTypeSpecTypeLoc(), diag::err_decimal_unsupported);
	// Result = Context.IntTy;
	// declarator.setInvalidType(true);

Decimal float impl #1

Are you sure you want to change the base?

Decimal float impl #1

Uh oh!

Conversation

shafik commented Nov 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zahiraam commented Nov 9, 2023

Uh oh!

tahonermann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tahonermann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!