[DFP] Initial changes to supprt DFP under APFloat and parse declarations #41

shafik · 2024-06-20T22:20:01Z

This change starts the base set of changes to support DFP in APFloat and allow us to parse a DFP decalration and examine the AST.

Since we don't have a DFP implementation I avoided implementating much beyond the basics for APFloat.

This change starts the base set of changes to support DFP in APFloat and allow us to parse a DFP decalration and examine the AST. Since we don't have a DFP implementation I avoided implementating much beyond the basics for APFloat.

shafik · 2024-06-20T22:25:59Z

Note, I stopped short of implementing DFP literals. At that point I would actually need to create a APFloat using DFPFloat and I think I would have had to implement a lot more and this was already getting large.

What I needed to get the AST working and needed to get the base APFloat changes going are a little independent but since we felt like these were the two things we wanted to see first I combined them together.

Note, I added semantics for BID and DPD and I added an enum to the fltSemantics to differentiate between base 2/10 and BID/DPD.

I also started out adding two sets of types for BID/DPD but I don't think we really want that. I think we just want DecimalFloat32/64/128Ty and we can differentiate BID/DPD solely with the fltSemantics. I am open to discussion here and on other aspects as well.

jcranmer-intel · 2024-06-25T20:51:26Z

llvm/include/llvm/ADT/APFloat.h

+  explicit DFPFloat(double d) { assert(false && "Not Implemented"); }
+  explicit DFPFloat(float f) { assert(false && "Not Implemented"); }


Tentatively going to say we shouldn't have these constructors at all. For DFP, an easy decimal string literal conversion is probably more useful than double or float (after all, you wouldn't want to have rounding errors trying to convert 0.1 to decimal32!)

jcranmer-intel · 2024-06-25T20:53:21Z

llvm/include/llvm/ADT/APFloat.h

+
+  cmpResult compareAbsoluteValue(const DFPFloat &) const;
+
+private:


Most of these private methods are probably unnecessary, especially the miscellany section.

Agreed. I recommend omitting them for now.

jcranmer-intel · 2024-06-25T20:54:19Z

llvm/include/llvm/IR/Type.h

+    DecimalFloat32TyID,
+    DecimalFloatDPD32TyID,
+    DecimalFloat64TyID,
+    DecimalFloatDPD64TyID,
+    DecimalFloat128TyID,
+    DecimalFloatDPD128TyID,


I had thought we had agreed that we were just going to have d32/d64/d128 types, with the bid/dpd being datalayout-level stuff?

Yes, that's my understanding too.

jcranmer-intel · 2024-06-25T20:55:47Z

llvm/include/llvm/ADT/APFloat.h

+  enum RadixAndFormat {
+    BaseTwo,
+    BaseTenBID,
+    BaseTenDPD
+  };


What's the rationale for combining radix and format into one enum?

It was a trial format but if we are not going to have separate types for BID/DPD then we don't need that anymore.

zahiraam · 2024-07-09T19:28:57Z

llvm/include/llvm/IR/Type.h

+    DecimalFloat32TyID,
+    DecimalFloatDPD32TyID,
+    DecimalFloat64TyID,
+    DecimalFloatDPD64TyID,
+    DecimalFloat128TyID,
+    DecimalFloatDPD128TyID,


Yes, that's my understanding too.

zahiraam · 2024-07-09T20:40:01Z

clang/include/clang/AST/ASTContext.h

@@ -1115,6 +1115,9 @@ class ASTContext : public RefCountedBase<ASTContext> {
  CanQualType HalfTy; // [OpenCL 6.1.1.1], ARM NEON
  CanQualType BFloat16Ty;
  CanQualType Float16Ty; // C11 extension ISO/IEC TS 18661-3
+  // ISO/IEC TS 18661-2:2015 c23 conditionally supported
+  CanQualType DecimalFloat32Ty, DecimalFloat64Ty, DecimalFloat128Ty;
+  CanQualType DecimalFloatDPD32Ty, DecimalFloatDPD64Ty, DecimalFloatDPD128Ty;


For consistency shouldn't we have DecimalFloatBID32Ty, DecimalFloatBID64Ty and DecimalFloatBID64Ty?

I think we should only have DecimalFloat32/64/128Ty with no prefix and differentiate the decoding via the fltSematics. WDYT?

I agree with @zahiraam; there should be just one set of types with behavior differentiated based on semantics.

These types are already present in the dfp branch.

zahiraam · 2024-07-17T19:28:29Z

llvm/lib/Support/APFloat.cpp

+static constexpr fltSemantics semDFP64BID = {385, -382, 16, 64, APFloatBase::BaseTenBID};
+static constexpr fltSemantics semDFP64DPD = {385, -382, 16, 64, APFloatBase::BaseTenDPD};
+static constexpr fltSemantics semDFP128BID = {6145, -6142, 34, 128, APFloatBase::BaseTenBID};
+static constexpr fltSemantics semDFP128DPD = {6145, -6142, 34, 128, APFloatBase::BaseTenDPD};


How did you get these values?

They came from the DFP proposal

I think it would be worth adding a comment that references the "Decimal interchange format parameters" table in C23 H.2.1, "Interchange floating types" and/or the same named table in IEEE 754-2008 3.6, "Interchange format parameters".

zahiraam · 2024-07-18T14:25:54Z

clang/lib/AST/Type.cpp

  if (const auto *CT = dyn_cast<ComplexType>(CanonicalType))
    return CT->getElementType()->isFloatingType();
  return false;
 }

+bool Type::isDecimalFloatType() const {
+  if (const auto *BT = dyn_cast<BuiltinType>(CanonicalType))


Can we call this function isDecimalFloatingType instead?

This function already exists in the dfp branch (with the name @zahiraam requested).

tahonermann · 2024-08-07T19:44:52Z

This PR is targeting branch main, but should be targeting branch dfp. It looks like we're working off of different branch points. Some of the changes included in this PR are already present in the dfp branch. Commits in the dfp branch can be seen here.

tahonermann

I think it would be useful to see the changes in a PR that is based on the existing dfp branch. That would make it easier to review just the changes that aren't already done.

I think it would also be useful to reduce the DFPFloat member functions to just those that we know will be needed as a starting point. That includes:

Those that correspond to an APFloat member function that uses APFLOAT_DISPATCH_ON_SEMANTICS.
Those that correspond to an APFloat member function that uses usesLayout<...> to dispatch calls.
Those that correspond to operations in IEEE 754-2008 section 5.7.2 (General operations).
New functions for operations in IEEE 754-2008 section 5.7.3 (Decimal operation).

It might make sense to add the needed changes to APFLOAT_DISPATCH_ON_SEMANTICS in this patch. We'll still need to audit all existing uses of usesLayout to identify other places that will require updates. We're also going to have to do something about getIEEE(); APFloat uses it to handle both IEEEFloat and DoubleAPFloat; there appears to be an assumption that DoubleAPFloat is always implemented in terms of (multiple values of) IEEEFloat; that won't be true for APFloat.

tahonermann · 2024-08-07T19:46:43Z

clang/include/clang/AST/ASTContext.h

@@ -1115,6 +1115,9 @@ class ASTContext : public RefCountedBase<ASTContext> {
  CanQualType HalfTy; // [OpenCL 6.1.1.1], ARM NEON
  CanQualType BFloat16Ty;
  CanQualType Float16Ty; // C11 extension ISO/IEC TS 18661-3
+  // ISO/IEC TS 18661-2:2015 c23 conditionally supported
+  CanQualType DecimalFloat32Ty, DecimalFloat64Ty, DecimalFloat128Ty;
+  CanQualType DecimalFloatDPD32Ty, DecimalFloatDPD64Ty, DecimalFloatDPD128Ty;


I agree with @zahiraam; there should be just one set of types with behavior differentiated based on semantics.

These types are already present in the dfp branch.

tahonermann · 2024-08-07T19:49:37Z

clang/lib/AST/Type.cpp

  if (const auto *CT = dyn_cast<ComplexType>(CanonicalType))
    return CT->getElementType()->isFloatingType();
  return false;
 }

+bool Type::isDecimalFloatType() const {
+  if (const auto *BT = dyn_cast<BuiltinType>(CanonicalType))


This function already exists in the dfp branch (with the name @zahiraam requested).

tahonermann · 2024-08-07T20:07:54Z

llvm/include/llvm/ADT/APFloat.h

+  bool operator==(const IEEEFloat &) const = delete;
+
+
+  cmpResult compare(const IEEEFloat &) const;
+
+
+  bool bitwiseIsEqual(const IEEEFloat &) const;


These cases of IEEEFloat should presumably be DFPFloat.

tahonermann · 2024-08-07T20:16:41Z

llvm/include/llvm/ADT/APFloat.h

+  bool isZero() const { return category == fcZero; }
+
+
+  bool isDenormal() const;


DFP types don't support the notion of a subnormal or denormal value. It looks like a function definition will either be needed for dispatch via APFLOAT_DISPATCH_ON_SEMANTICS in APFloat::isDenormal(), or the latter function will need to be special cased for IEEEFloat and DoubleAPFloat. Either approach seems reasonable to me. If we do provide a definition, then we need to decide whether it should just return false or should call llvm::report_fatal_error() or similar.

tahonermann · 2024-08-07T20:36:02Z

llvm/include/llvm/ADT/APFloat.h

+
+  cmpResult compareAbsoluteValue(const DFPFloat &) const;
+
+private:


Agreed. I recommend omitting them for now.

jcranmer-intel · 2024-08-14T21:18:59Z

I think it would also be useful to reduce the DFPFloat member functions to just those that we know will be needed as a starting point. That includes:

To make forward progress on LLVM IR, the main things I need are the ability to take a DFPFloat to/from a decimal string, as well as the operations in 5.5.2 (go between a DFPFloat and the binary bits for BID/DPD formats).

shafik requested review from tahonermann, jcranmer-intel and zahiraam June 20, 2024 22:20

jcranmer-intel reviewed Jun 25, 2024

View reviewed changes

zahiraam reviewed Jul 17, 2024

View reviewed changes

zahiraam reviewed Jul 18, 2024

View reviewed changes

tahonermann requested changes Aug 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DFP] Initial changes to supprt DFP under APFloat and parse declarations #41

[DFP] Initial changes to supprt DFP under APFloat and parse declarations #41

shafik commented Jun 20, 2024

shafik commented Jun 20, 2024

jcranmer-intel Jun 25, 2024

jcranmer-intel Jun 25, 2024

tahonermann Aug 7, 2024

jcranmer-intel Jun 25, 2024

zahiraam Jul 9, 2024

jcranmer-intel Jun 25, 2024

shafik Aug 28, 2024

zahiraam Jul 9, 2024

zahiraam Jul 9, 2024

zahiraam Jul 22, 2024

tahonermann Aug 7, 2024

zahiraam Jul 17, 2024

shafik Aug 27, 2024

tahonermann Aug 28, 2024

zahiraam Jul 18, 2024

tahonermann Aug 7, 2024

tahonermann commented Aug 7, 2024 •

edited

Loading

tahonermann left a comment

tahonermann Aug 7, 2024

tahonermann Aug 7, 2024

tahonermann Aug 7, 2024

tahonermann Aug 7, 2024

tahonermann Aug 7, 2024

jcranmer-intel commented Aug 14, 2024

		explicit DFPFloat(double d) { assert(false && "Not Implemented"); }
		explicit DFPFloat(float f) { assert(false && "Not Implemented"); }


		cmpResult compareAbsoluteValue(const DFPFloat &) const;

		private:

		bool isZero() const { return category == fcZero; }


		bool isDenormal() const;

[DFP] Initial changes to supprt DFP under APFloat and parse declarations #41

Are you sure you want to change the base?

[DFP] Initial changes to supprt DFP under APFloat and parse declarations #41

Conversation

shafik commented Jun 20, 2024

shafik commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tahonermann commented Aug 7, 2024 • edited Loading

tahonermann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcranmer-intel commented Aug 14, 2024

tahonermann commented Aug 7, 2024 •

edited

Loading