[llvm] [DFP] Add support for decimal floating point IR types. #33

zahiraam · 2023-10-24T21:42:21Z

Add decimal32, decimal64 and decimal128 IR types.
Still to be done:
APFloat
Constant Folding

Most of the comments were in this (now)closed PR thread. llvm#69718

jcranmer-intel

I've noticed that DataLayout is also missing a way to describe the ABI layout of decimal floating point types.

jcranmer-intel · 2023-10-25T15:37:47Z

llvm/include/llvm/IR/Type.h

+    case Decimal32TyID:
+    case Decimal64TyID:
+    case Decimal128TyID:


I don't think the decimal types should qualify as isIEEELikeFPTy--one of the requirements is that they don't have noncanonical values, and decimal FP types do have noncanonical values (like x86_fp80.

That makes sense.
IsFloatingType should then become:

bool isFloatingPointTy() const { return isIEEELikeFPTy() || getTypeID() == X86_FP80TyID || getTypeID() == PPC_FP128TyID || getTypeID() == Decimal32TyID || getTypeID() == Decimal64TyID || getTypeID() == Decimal128TyID; }

May be we should have something like isDFPFLoatingType to check only for DFP?

llvm/include/llvm/IR/Type.h

jcranmer-intel · 2023-10-25T15:55:41Z

llvm/lib/IR/Type.cpp

+  // See comment in the declaration of Type::getFPMantissaWidth();
+  // it returns -1 if the FP type does not have a stable mantissa.


I think it's worth adding a clarification in Type.h documentation that getFPMantissaWidth returns -1 for decimal floating point types.

From looking at the existing calls to this method, it's safer to return -1 here than the number of decimal digits that decimal FP types can store. The main uses of this function are a) what's the largest integer that can be precisely stored in this FP types and b) checks for when double rounding is known to not be an issue, and in both cases, bailing out for decimal FP types is the correct thing to do.

Should this function support DFP types at all? Perhaps we should just assert(!isDecimalFloatingPointTy()).

Are we not going to need to compute the mantissa for a DFP, you think?

I think we will, but whether that number makes sense in the context of the existing callers is unclear to me. I would expect existing callers to interpret the result in a context that assumes binary FP and therefore draw incorrect inclusions about precision. Perhaps this is a function that it would make sense to split for binary FP and decimal FP types.

Ok. Creating a getDFPMantissaWidth.

Should this function support DFP types at all? Perhaps we should just assert(!isDecimalFloatingPointTy()).

I think -1 works better, because the strong temptation is to call this on things that support isFloatingPointTy(), not necessarily isBinaryFloatingPointTy() (which, btw, is probably another method that should exist).

Sounds good. Should we rename the existing function to getBFPMantissaWidth() to make it explicit that it is only for binary FP?

I'd shy away from renaming for now; this is a topic that is best visited during the ultimate community code review rather than among ourselves right now.

jcranmer-intel · 2023-10-25T20:30:32Z

Another thought that I just had was that maybe datalayout should also indicate whether or not BID or DPD encoding is being used... or maybe they should just be different decimal FP types in the IR altogether?

tahonermann · 2023-10-25T22:45:08Z

llvm/include/llvm-c/Core.h

+  LLVMDecimal32TypeKind, /**< 32 bit decimal floating point type */
  LLVMDoubleTypeKind,    /**< 64 bit floating point type */
+  LLVMDecimal64TypeKind, /**< 64 bit decimal floating point type */
  LLVMX86_FP80TypeKind,  /**< 80 bit floating point type (X87) */
  LLVMFP128TypeKind,     /**< 128 bit floating point type (112-bit mantissa)*/
  LLVMPPC_FP128TypeKind, /**< 128 bit floating point type (two 64-bits) */
+  LLVMDecimal128TypeKind,/**< 128 bit decimal floating point type */


I'm guessing this doesn't matter (I see LLVMBFloatTypeKind was added further below), but I'm more inclined to keep the DFP types together and separated from the binary FP types here and elsewhere. Though they are floating-point types, I expect that we'll have to branch handling for binary vs decimal FP types in many places.

You mean creating a separate enum for them? Or you mean in the same enum but together?

I mean keep them in the same enumeration, but have the DFP types grouped together rather than distributed among the binary FP types. Likewise elsewhere throughout the patch.

I completely missed this earlier, but you should only append values to this struct, never add in the middle, as we want to preserve the values of all the enum consts in this and other structs in include/llvm-c.

llvm/include/llvm/IR/Type.h

tahonermann · 2023-10-25T22:55:29Z

llvm/include/llvm/IR/Type.h

+    case Decimal32TyID:
+    case Decimal64TyID:
+    case Decimal128TyID:


tahonermann · 2023-10-26T17:31:49Z

llvm/lib/AsmParser/LLLexer.cpp

+///    FIXME: I have temporarily added these prefixes temporarly to mimic
+///    the DFP attributes in C++. But this is just a placeholder for
+///    the real prefix.


Suggested change

/// FIXME: I have temporarily added these prefixes temporarly to mimic

/// the DFP attributes in C++. But this is just a placeholder for

/// the real prefix.

/// FIXME: I have temporarily added these prefixes to mimic

/// the DFP attributes in C++. But this is just a placeholder for

/// the real prefix.

tahonermann · 2023-10-26T17:44:40Z

llvm/lib/AsmParser/LLLexer.cpp

+///    the DFP attributes in C++. But this is just a placeholder for
+///    the real prefix.
+///    HexDecimal32Constant 0xSD[0-9A-Fa-f]+
+///    HexDecimal64Constant 0xDD[0-9A-Fa-f]+


The 0xDD prefix looks like it introduces an ambiguity with the existing HexFPConstant. The literal suffixes (DF, DD, and DL) have the same problem.

It looks like we can probably omit this support; the C23 draft, in 6.4.4.2, Floating constants, p2, states that DFP suffixes shall not be used with hexadecimal floating constants.

Ok. So this can be removed?

I think so, yes.

We need some way of precisely rendering decimal floating-point constants in LLVM IR... except, since it's decimal, we can just use regular decimal floating-point for that.

The existing code uses a double-sized APFloat for regular FP constants, which is not going to work for decimal FP constants, however.

tahonermann · 2023-10-26T17:49:29Z

llvm/lib/IR/Type.cpp

+  // See comment in the declaration of Type::getFPMantissaWidth();
+  // it returns -1 if the FP type does not have a stable mantissa.


Should this function support DFP types at all? Perhaps we should just assert(!isDecimalFloatingPointTy()).

tahonermann · 2023-10-26T18:04:04Z

Another thought that I just had was that maybe datalayout should also indicate whether or not BID or DPD encoding is being used... or maybe they should just be different decimal FP types in the IR altogether?

I don't think there is a need for the encoding to be reflected in the type system. The existing library conversion routines don't provide any differentiation. See https://github.com/gcc-mirror/gcc/blob/master/libdecnumber/bid/bid-dpd.h for example. Within a single translation unit, I would expect only one encoding to ever be supported by core language operations. It might be necessary to have that encoding somehow reflected at the TU level though so that a backend can map the IR intrinsic operations to the correct instructions or run-time library function. We could entertain defining distinct sets of IR intrinsics (e.g., dfp_add_bid and dfp_add_dpd), but I suspect we should avoid doing that if possible.

zahiraam · 2023-10-26T18:31:40Z

Another thought that I just had was that maybe datalayout should also indicate whether or not BID or DPD encoding is being used... or maybe they should just be different decimal FP types in the IR altogether?

I don't think there is a need for the encoding to be reflected in the type system. The existing library conversion routines don't provide any differentiation. See https://github.com/gcc-mirror/gcc/blob/master/libdecnumber/bid/bid-dpd.h for example. Within a single translation unit, I would expect only one encoding to ever be supported by core language operations. It might be necessary to have that encoding somehow reflected at the TU level though so that a backend can map the IR intrinsic operations to the correct instructions or run-time library function. We could entertain defining distinct sets of IR intrinsics (e.g., dfp_add_bid and dfp_add_dpd), but I suspect we should avoid doing that if possible.

I agree that we shouldn't be switching from one encoding to another in a TU. In this case we don't need the encoding to be reflected in the type. I think that was the only reason we would have had to make the type know about the encoding, right?

How would we set the encoding at the TU level?

tahonermann · 2023-10-26T18:35:54Z

How would we set the encoding at the TU level?

Good question. I'm not nearly familiar enough with the LLVM IR to have a perspective on this. Perhaps @jcranmer-intel has opinions to share.

tahonermann · 2023-10-26T18:42:17Z

Ah, this comment from @jcranmer-intel sounds right to me:

Another thought that I just had was that maybe datalayout should also indicate whether or not BID or DPD encoding is being used.

I'm assuming datalayout here refers to the following directive that can be seen in the emitted IR for compilation with -emit-llvm.

target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"

jcranmer-intel · 2023-10-26T21:30:57Z

I don't think there is a need for the encoding to be reflected in the type system. The existing library conversion routines don't provide any differentiation. See https://github.com/gcc-mirror/gcc/blob/master/libdecnumber/bid/bid-dpd.h for example. Within a single translation unit, I would expect only one encoding to ever be supported by core language operations. It might be necessary to have that encoding somehow reflected at the TU level though so that a backend can map the IR intrinsic operations to the correct instructions or run-time library function. We could entertain defining distinct sets of IR intrinsics (e.g., dfp_add_bid and dfp_add_dpd), but I suspect we should avoid doing that if possible.

Yeah, after further thought, I think in IR terms, it's better to handle DPD/BID as an endianness-like decision: a single, global flag. (I can't see anyone putting together a system where some types are DPD and others are BID.)

The question is a little more salient on supporting decimal floats in APFloat, because it really matters for bitcasting to/from APInt, but the question is otherwise ignorable. And even then, designing an APFloat-like class that only cares about DPD/BID on a bitcastToAPInt(DecFormat) method isn't difficult. It's only difficult if you're trying to use it in a situation where decimal floats are themselves an APFloat instance, and thus the format parameter has to become optional (for ease of use with binary floats).

tahonermann · 2023-10-26T21:39:33Z

The question is a little more salient on supporting decimal floats in APFloat

The intention so far has been to introduce a APDecimalFloat; @shafik has already done work on this.

tahonermann · 2023-10-26T21:53:23Z

Yeah, after further thought, I think in IR terms, it's better to handle DPD/BID as an endianness-like decision: a single, global flag.

That seems reasonable, but I'm unfamiliar here. I don't see an obvious global flag for endianness in the LLVM IR emitted for a simple compilation with -emit-llvm. Looking at code, I do see a BigEndian data member in the llvm::DataLayout class and, assuming I'm reading the code right, it looks like that corresponds to a e or E specifier in the datalayout directive. If so, great! I agree it sounds like adding an indicator there sounds right.

(I can't see anyone putting together a system where some types are DPD and others are BID.)

That matches my intuition as well.

jcranmer-intel · 2023-10-27T13:46:50Z

Looking at code, I do see a BigEndian data member in the llvm::DataLayout class and, assuming I'm reading the code right, it looks like that corresponds to a e or E specifier in the datalayout directive. If so, great!

You are correct in reading that code.

changes.

jcranmer-intel · 2023-10-31T19:38:32Z

llvm/include/llvm/IR/Type.h

@@ -336,9 +354,12 @@ class Type {

  /// Return the width of the mantissa of this type. This is only valid on
  /// floating-point types. If the FP type does not have a stable mantissa (e.g.
-  /// ppc long double), this method returns -1.
+  /// ppc long double), or if the type is a decimal floating point this method
+  /// returns -1.


If we're going with asserting in this method on decimal FP types, this documentation change is incorrect.

llvm/include/llvm/IR/Type.h

jcranmer-intel · 2023-10-31T19:42:57Z

llvm/lib/IR/Type.cpp

+    return VTy->getElementType()->getDFPMantissaWidth();
+  assert(isDecimalFloatingPointTy() && "Not a decimal floating point type!");
+  // For both BID and DPD the width of the mantissa varies and is dependent
+  // on the combination fields.


If you've got a distinction between binary floating-point and decimal floating-point types for this method, then the decimal version probably should return the width in decimal digits as opposed to integer bits. A method that returns exclusively -1 for all the types it can legally be queried on isn't a particularly useful method, IMHO.

Agreed! This is really just a place holder. The width of the mantissa is dependent on the combination fields and the encoding, I am not quite sure at this point how to compute that.

The values are specified in "Decimal interchange format parameters" table in section X.2.1 of WG14 N2601 as follows:

Decimal32: 7

Decimal64: 16

Decimal128: 34

My understanding is that the values are not encoding dependent.

Thanks. I was looking for that! Why do they call it "Decimal interchange formal parameters"? Doesn't make any sense to me.

You know us specification writers. If we can find a way to specify it in a way that you wouldn't expect, you can rest assured that is exactly what we'll do! ;)

The formats are intended to be portable, so that is where "interchange" comes from. "formal parameters" is there because the formats are algorithmic and parameterized on the specified values.

The values are specified in "Decimal interchange format parameters" table in section X.2.1 of WG14 N2601 as follows:

You should probably use H.2.1 of C23, given that the TS has been incorporated into C23. (Although, very confusingly, the table is mislabeled when copied into C23).

Thanks. I was looking for that! Why do they call it "Decimal interchange formal parameters"? Doesn't make any sense to me.

It's copy-pasted eventually from IEEE 754. Floating point types there technically aren't bound to any specific representation, and are instead based entirely on how they model parameters of b (base), p (bits/digits), Emax (largest finite exponent). The interchange formats are the formats that have a specified layout, and 7 specific layouts are given (for binary16/32/64/128 and decimal32/64/128). (They also have an additional parameter compared to the computation formats, which is the number of bits in the storage format.)

In practice, all floating point types are represented using the IEEE 754 interchange format layout on any hardware made after I was born, so "interchange format" is really synonymous with "floating point format"

Thank you. I can see that even the table that Tom mentioned is in there!

tahonermann

A few minor comments.

Could you also update CodeGenTypes::ConvertType(QualType T) in clang/lib/CodeGen/CodeGenTypes.cpp to map BuiltinType::DecimalFloatNN to the IR types? Search for report_fatal_error.

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

llvm/include/llvm-c/Core.h

+LLVMTypeRef LLVMDecimal32Type(void);
 LLVMTypeRef LLVMDoubleType(void);
+LLVMTypeRef LLVMDecimal64Type(void);
 LLVMTypeRef LLVMX86FP80Type(void);
 LLVMTypeRef LLVMFP128Type(void);
 LLVMTypeRef LLVMPPCFP128Type(void);
+LLVMTypeRef LLVMDecimal128Type(void);


Please separate the DFP related declarations from the other FP ones here.

tahonermann · 2023-11-01T21:15:52Z

llvm/include/llvm/IR/Type.h

+    Decimal32TyID, ///< 32-bit decimal floating point type
    FloatTyID,     ///< 32-bit floating point type
+    Decimal64TyID, ///< 64-bit decimal floating point type
    DoubleTyID,    ///< 64-bit floating point type
    X86_FP80TyID,  ///< 80-bit floating point type (X87)
+    Decimal128TyID, ///< 128-bit decimal floating point type


Please separate the DFP related declarations from the other FP ones here.

tahonermann · 2023-11-01T21:42:15Z

llvm/include/llvm/IR/Type.h

+  /// Return the width of the mantissa of a decimal floating-point type.
+  int getDFPMantissaWidth() const;
+


Let's clarify the documentation for both getFPMantissaWidth() and getDFPMantissaWidth(). For the former, the value returned is the precision in bits. For the latter, the value is the precision in digits. The X.2.1, "Interchange floating types" section of WG14 N2601 specifies these values.

In order to avoid confusion, I think we should name these functions differently. Joshua has requested that we not rename getFPMantissaWidth() now and that is reasonable. Perhaps name the new function getDFPPrecisionInDigits()?

tahonermann · 2023-11-01T21:44:43Z

llvm/lib/IR/Type.cpp

+    return VTy->getElementType()->getDFPMantissaWidth();
+  assert(isDecimalFloatingPointTy() && "Not a decimal floating point type!");
+  // For both BID and DPD the width of the mantissa varies and is dependent
+  // on the combination fields.


The values are specified in "Decimal interchange format parameters" table in section X.2.1 of WG14 N2601 as follows:

Decimal32: 7

Decimal64: 16

Decimal128: 34

My understanding is that the values are not encoding dependent.

tahonermann

I added a few more minor comments.

Changes to llvm::LLLexer::LexDigitOrNegative() and llvm::LLLexer::LexPositive() are also needed to lex DFP constant values, but I'm happy for that to be done in a separate review if that is your preference. It looks like we'll have to introduce a suffix notation or similar there in order to recognize DFP values. That will also presumably require corresponding changes to LLVM IR text generation functions.

llvm/include/llvm/IR/Type.h

tahonermann · 2023-11-07T15:31:30Z

llvm/include/llvm/IR/Type.h

+  /// Return the width of the mantissa of a decimal floating-point type, in
+  /// digits. See table in section X.2.1 of WG14 N2601.


Suggested change

/// Return the width of the mantissa of a decimal floating-point type, in

/// digits. See table in section X.2.1 of WG14 N2601.

/// Return the precision in digits of a decimal floating-point type

/// per the "Decimal interchange format parameters" table of

/// C23 annex H.2.1, "Interchange floating types".

Let's reference the (not officially complete but close enough) C23 standard here as Joshua suggested.

tahonermann · 2023-11-07T15:35:26Z

llvm/lib/AsmParser/LLLexer.cpp

@@ -987,6 +990,7 @@ lltok::Kind LLLexer::LexIdentifier() {
 lltok::Kind LLLexer::Lex0x() {
  CurPtr = TokStart + 2;

+  // TODO: Handle the DFP type here.


Suggested change

// TODO: Handle the DFP type here.

Per the C23 draft, section 6.4.4.2, Floating constants, p2, hexadecimal floating constants are not supported for DFP types. I think this comment can be removed.

Then I don't understand the text that comes in p5?

p5 applies to FP constants in general where as p2 restricts applicability of hexadecimal-floating-constant to non-DFP types.

tahonermann · 2023-11-07T15:40:27Z

llvm/lib/AsmParser/LLLexer.cpp

+    // FIXME: Need to add code here for DFP, using the APDecimalFloat.
+


Suggested change

// FIXME: Need to add code here for DFP, using the APDecimalFloat.

Per the C23 draft, section 6.4.4.2, Floating constants, p2, hexadecimal floating constants are not supported for DFP types. I think this comment can be removed.

tahonermann · 2023-11-07T15:42:26Z

llvm/lib/IR/Type.cpp

+  if (auto *VTy = dyn_cast<VectorType>(this))
+    return VTy->getElementType()->getDFPPrecisionInDigits();
+  assert(isDecimalFloatingPointTy() && "Not a decimal floating point type!");
+  // Precision values following the table in section X.2.1 of WG14 N2601.


Suggested change

// Precision values following the table in section X.2.1 of WG14 N2601.

// Precision values following the "Decimal interchange format parameters"

/// table of C23 annex H.2.1, "Interchange floating types".

Can this comment change be applied, please?

zahiraam · 2023-11-07T16:10:30Z

I added a few more minor comments.

Changes to llvm::LLLexer::LexDigitOrNegative() and llvm::LLLexer::LexPositive() are also needed to lex DFP constant values, but I'm happy for that to be done in a separate review if that is your preference. It looks like we'll have to introduce a suffix notation or similar there in order to recognize DFP values. That will also presumably require corresponding changes to LLVM IR text generation functions.

Yes. If you don't mind, I would prefer to do that in another PR.

tahonermann · 2023-11-07T16:39:55Z

Yes. If you don't mind, I would prefer to do that in another PR.

Sounds good. I filed issue #35 to do so.

tahonermann · 2023-11-07T19:17:32Z

Ah, this comment from @jcranmer-intel sounds right to me:

Another thought that I just had was that maybe datalayout should also indicate whether or not BID or DPD encoding is being used.

I'm assuming datalayout here refers to the following directive that can be seen in the emitted IR for compilation with -emit-llvm.
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"

Per previous comments, an update to datalayout is needed to reflect the DFP encoding. I filed issue #36 to follow up on this. I see no need to address this as part of the current PR.

Ariel-Burton · 2023-11-09T22:31:39Z

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

+      break;
+    case Type::Decimal128TyID:
+      Code = bitc::TYPE_CODE_DECIMAL128;
+      break;


I think this chunk should be positioned with the other floating point types, at around line 2310,

Ariel-Burton · 2023-11-09T22:45:31Z

llvm/lib/IR/Core.cpp

+  case Type::Decimal32TyID:
+    return LLVMDecimal32TypeKind;
+  case Type::Decimal64TyID:
+    return LLVMDecimal64TypeKind;


Again, shift nearer the other floating point types.

Ariel-Burton · 2023-11-09T23:16:51Z

llvm/lib/IR/Type.cpp

@@ -191,6 +194,9 @@ TypeSize Type::getPrimitiveSizeInBits() const {
    assert(!ETS.isScalable() && "Vector type should have fixed-width elements");
    return {ETS.getFixedValue() * EC.getKnownMinValue(), EC.isScalable()};
  }
+  case Decimal32TyID: return TypeSize::Fixed(32);
+  case Decimal64TyID: return TypeSize::Fixed(64);
+  case Decimal128TyID: return TypeSize::Fixed(128);


shift up to be with the other scalars.

Ariel-Burton · 2023-11-09T23:21:17Z

llvm/lib/IR/Type.cpp

+  if (getTypeID() == Decimal32TyID) return 7;
+  if (getTypeID() == Decimal64TyID) return 16;
+  if (getTypeID() == Decimal128TyID) return 34;
+  assert("unknown decimal floating point type");


What's the idiom here? Isn't the condition in the assert always logical true?

Suggested change

assert("unknown decimal floating point type");

report_fatal_error("unknown decimal floating point type");

Good catch! I recommend using report_fatal_error() here (include llvm/Support/ErrorHandling.h).

In which case, since report_fatal_error() is declared [[noreturn]], the following return statement can be removed as well.

tahonermann

This all looks good to me. I noted one other suggested change to a comment to be applied. Please just apply that as part of pushing the change. Thank you for following through on all these review iterations!

tahonermann · 2023-11-14T20:54:09Z

llvm/lib/IR/Type.cpp

+  if (auto *VTy = dyn_cast<VectorType>(this))
+    return VTy->getElementType()->getDFPPrecisionInDigits();
+  assert(isDecimalFloatingPointTy() && "Not a decimal floating point type!");
+  // Precision values following the table in section X.2.1 of WG14 N2601.


Can this comment change be applied, please?

shafik · 2023-11-14T21:42:48Z

llvm/lib/IR/Type.cpp

+  case Decimal32TyID:  return getDecimal32Ty(C);
+  case Decimal64TyID:  return getDecimal64Ty(C);


Suggested change

case Decimal32TyID: return getDecimal32Ty(C);

case Decimal64TyID: return getDecimal64Ty(C);

case Decimal32TyID : return getDecimal32Ty(C);

case Decimal64TyID : return getDecimal64Ty(C);

Really a nit to get the alignment of the :

shafik

LGTM

zahiraam · 2023-11-14T22:29:01Z

Thanks for all the reviews.

@0

…lvm#80904)" This reverts commit b1ac052. This commit breaks coroutine splitting for non-swift calling convention functions. In this example: ```ll ; ModuleID = 'repro.ll' source_filename = "stdlib/test/runtime/test_llcl.mojo" target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128" target triple = "x86_64-unknown-linux-gnu" @0 = internal constant { i32, i32 } { i32 trunc (i64 sub (i64 ptrtoint (ptr @craSH to i64), i64 ptrtoint (ptr getelementptr inbounds ({ i32, i32 }, ptr @0, i32 0, i32 1) to i64)) to i32), i32 64 } define dso_local void @af_suspend_fn(ptr %0, i64 %1, ptr %2) #0 { ret void } define dso_local void @craSH(ptr %0) #0 { %2 = call token @llvm.coro.id.async(i32 64, i32 8, i32 0, ptr @0) %3 = call ptr @llvm.coro.begin(token %2, ptr null) %4 = getelementptr inbounds { ptr, { ptr, ptr }, i64, { ptr, i1 }, i64, i64 }, ptr poison, i32 0, i32 0 %5 = call ptr @llvm.coro.async.resume() store ptr %5, ptr %4, align 8 %6 = call { ptr, ptr, ptr } (i32, ptr, ptr, ...) @llvm.coro.suspend.async.sl_p0p0p0s(i32 0, ptr %5, ptr @ctxt_proj_fn, ptr @af_suspend_fn, ptr poison, i64 -1, ptr poison) ret void } define dso_local ptr @ctxt_proj_fn(ptr %0) #0 { ret ptr %0 } ; Function Attrs: nomerge nounwind declare { ptr, ptr, ptr } @llvm.coro.suspend.async.sl_p0p0p0s(i32, ptr, ptr, ...) #1 ; Function Attrs: nounwind declare token @llvm.coro.id.async(i32, i32, i32, ptr) #2 ; Function Attrs: nounwind declare ptr @llvm.coro.begin(token, ptr writeonly) #2 ; Function Attrs: nomerge nounwind declare ptr @llvm.coro.async.resume() #1 attributes #0 = { "target-features"="+adx,+aes,+avx,+avx2,+bmi,+bmi2,+clflushopt,+clwb,+clzero,+crc32,+cx16,+cx8,+f16c,+fma,+fsgsbase,+fxsr,+invpcid,+lzcnt,+mmx,+movbe,+mwaitx,+pclmul,+pku,+popcnt,+prfchw,+rdpid,+rdpru,+rdrnd,+rdseed,+sahf,+sha,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+sse4a,+ssse3,+vaes,+vpclmulqdq,+wbnoinvd,+x87,+xsave,+xsavec,+xsaveopt,+xsaves" } attributes #1 = { nomerge nounwind } attributes #2 = { nounwind } ``` This verifier crashes after the `coro-split` pass with ``` cannot guarantee tail call due to mismatched parameter counts musttail call void @af_suspend_fn(ptr poison, i64 -1, ptr poison) LLVM ERROR: Broken function PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace. Stack dump: 0. Program arguments: opt ../../../reduced.ll -O0 #0 0x00007f1d89645c0e __interceptor_backtrace.part.0 /build/gcc-11-XeT9lY/gcc-11-11.4.0/build/x86_64-linux-gnu/libsanitizer/asan/../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:4193:28 #1 0x0000556d94d254f7 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/ubuntu/modular/third-party/llvm-project/llvm/lib/Support/Unix/Signals.inc:723:22 #2 0x0000556d94d19a2f llvm::sys::RunSignalHandlers() /home/ubuntu/modular/third-party/llvm-project/llvm/lib/Support/Signals.cpp:105:20 #3 0x0000556d94d1aa42 SignalHandler(int) /home/ubuntu/modular/third-party/llvm-project/llvm/lib/Support/Unix/Signals.inc:371:36 #4 0x00007f1d88e42520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520) #5 0x00007f1d88e969fc __pthread_kill_implementation ./nptl/pthread_kill.c:44:76 #6 0x00007f1d88e969fc __pthread_kill_internal ./nptl/pthread_kill.c:78:10 #7 0x00007f1d88e969fc pthread_kill ./nptl/pthread_kill.c:89:10 #8 0x00007f1d88e42476 gsignal ./signal/../sysdeps/posix/raise.c:27:6 #9 0x00007f1d88e287f3 abort ./stdlib/abort.c:81:7 #10 0x0000556d8944be01 std::vector<llvm::json::Value, std::allocator<llvm::json::Value>>::size() const /usr/include/c++/11/bits/stl_vector.h:919:40 #11 0x0000556d8944be01 bool std::operator==<llvm::json::Value, std::allocator<llvm::json::Value>>(std::vector<llvm::json::Value, std::allocator<llvm::json::Value>> const&, std::vector<llvm::json::Value, std::allocator<llvm::json::Value>> const&) /usr/include/c++/11/bits/stl_vector.h:1893:23 #12 0x0000556d8944be01 llvm::json::operator==(llvm::json::Array const&, llvm::json::Array const&) /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/Support/JSON.h:572:69 #13 0x0000556d8944be01 llvm::json::operator==(llvm::json::Value const&, llvm::json::Value const&) (.cold) /home/ubuntu/modular/third-party/llvm-project/llvm/lib/Support/JSON.cpp:204:28 #14 0x0000556d949ed2bd llvm::report_fatal_error(char const*, bool) /home/ubuntu/modular/third-party/llvm-project/llvm/lib/Support/ErrorHandling.cpp:82:70 #15 0x0000556d8e37e876 llvm::SmallVectorBase<unsigned int>::size() const /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/ADT/SmallVector.h:91:32 #16 0x0000556d8e37e876 llvm::SmallVectorTemplateCommon<llvm::DiagnosticInfoOptimizationBase::Argument, void>::end() /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/ADT/SmallVector.h:282:41 #17 0x0000556d8e37e876 llvm::SmallVector<llvm::DiagnosticInfoOptimizationBase::Argument, 4u>::~SmallVector() /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/ADT/SmallVector.h:1215:24 #18 0x0000556d8e37e876 llvm::DiagnosticInfoOptimizationBase::~DiagnosticInfoOptimizationBase() /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/IR/DiagnosticInfo.h:413:7 #19 0x0000556d8e37e876 llvm::DiagnosticInfoIROptimization::~DiagnosticInfoIROptimization() /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/IR/DiagnosticInfo.h:622:7 #20 0x0000556d8e37e876 llvm::OptimizationRemark::~OptimizationRemark() /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/IR/DiagnosticInfo.h:689:7 #21 0x0000556d8e37e876 operator() /home/ubuntu/modular/third-party/llvm-project/llvm/lib/Transforms/Coroutines/CoroSplit.cpp:2213:14 #22 0x0000556d8e37e876 emit<llvm::CoroSplitPass::run(llvm::LazyCallGraph::SCC&, llvm::CGSCCAnalysisManager&, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&)::<lambda()> > /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/Analysis/OptimizationRemarkEmitter.h:83:12 #23 0x0000556d8e37e876 llvm::CoroSplitPass::run(llvm::LazyCallGraph::SCC&, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>&, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) /home/ubuntu/modular/third-party/llvm-project/llvm/lib/Transforms/Coroutines/CoroSplit.cpp:2212:13 #24 0x0000556d8c36ecb1 llvm::detail::PassModel<llvm::LazyCallGraph::SCC, llvm::CoroSplitPass, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&>::run(llvm::LazyCallGraph::SCC&, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>&, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/IR/PassManagerInternal.h:91:3 #25 0x0000556d91c1a84f llvm::PassManager<llvm::LazyCallGraph::SCC, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&>::run(llvm::LazyCallGraph::SCC&, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>&, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) /home/ubuntu/modular/third-party/llvm-project/llvm/lib/Analysis/CGSCCPassManager.cpp:90:12 #26 0x0000556d8c3690d1 llvm::detail::PassModel<llvm::LazyCallGraph::SCC, llvm::PassManager<llvm::LazyCallGraph::SCC, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&>, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&>::run(llvm::LazyCallGraph::SCC&, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>&, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/IR/PassManagerInternal.h:91:3 #27 0x0000556d91c2162d llvm::ModuleToPostOrderCGSCCPassAdaptor::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) /home/ubuntu/modular/third-party/llvm-project/llvm/lib/Analysis/CGSCCPassManager.cpp:278:18 #28 0x0000556d8c369035 llvm::detail::PassModel<llvm::Module, llvm::ModuleToPostOrderCGSCCPassAdaptor, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/IR/PassManagerInternal.h:91:3 #29 0x0000556d9457abc5 llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/IR/PassManager.h:247:20 #30 0x0000556d8e30979e llvm::CoroConditionalWrapper::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) /home/ubuntu/modular/third-party/llvm-project/llvm/lib/Transforms/Coroutines/CoroConditionalWrapper.cpp:19:74 #31 0x0000556d8c365755 llvm::detail::PassModel<llvm::Module, llvm::CoroConditionalWrapper, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/IR/PassManagerInternal.h:91:3 #32 0x0000556d9457abc5 llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/IR/PassManager.h:247:20 #33 0x0000556d89818556 llvm::SmallPtrSetImplBase::isSmall() const /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:196:33 #34 0x0000556d89818556 llvm::SmallPtrSetImplBase::~SmallPtrSetImplBase() /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:84:17 #35 0x0000556d89818556 llvm::SmallPtrSetImpl<llvm::AnalysisKey*>::~SmallPtrSetImpl() /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:321:7 #36 0x0000556d89818556 llvm::SmallPtrSet<llvm::AnalysisKey*, 2u>::~SmallPtrSet() /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:427:7 #37 0x0000556d89818556 llvm::PreservedAnalyses::~PreservedAnalyses() /home/ubuntu/modular/third-party/llvm-project/llvm/include/llvm/IR/Analysis.h:109:7 #38 0x0000556d89818556 llvm::runPassPipeline(llvm::StringRef, llvm::Module&, llvm::TargetMachine*, llvm::TargetLibraryInfoImpl*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::StringRef, llvm::ArrayRef<llvm::PassPlugin>, llvm::ArrayRef<std::function<void (llvm::PassBuilder&)>>, llvm::opt_tool::OutputKind, llvm::opt_tool::VerifierKind, bool, bool, bool, bool, bool, bool, bool) /home/ubuntu/modular/third-party/llvm-project/llvm/tools/opt/NewPMDriver.cpp:532:10 #39 0x0000556d897e3939 optMain /home/ubuntu/modular/third-party/llvm-project/llvm/tools/opt/optdriver.cpp:737:27 #40 0x0000556d89455461 main /home/ubuntu/modular/third-party/llvm-project/llvm/tools/opt/opt.cpp:25:33 #41 0x00007f1d88e29d90 __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16 #42 0x00007f1d88e29e40 call_init ./csu/../csu/libc-start.c:128:20 #43 0x00007f1d88e29e40 __libc_start_main ./csu/../csu/libc-start.c:379:5 #44 0x0000556d897b6335 _start (/home/ubuntu/modular/.derived/third-party/llvm-project/build-relwithdebinfo-asan/bin/opt+0x150c335) Aborted (core dumped)

zahiraam added 4 commits October 24, 2023 14:39

Add support for DFP IR type.

002b2f9

Fixed BitCodeFormat.rst format.

bf521f6

Addressed review comments.

49f4453

Addressed review comments.

b10d51e

zahiraam changed the title ~~Add support for DFP IR type.~~ [llvm] Add support for DFP IR type. Oct 24, 2023

zahiraam changed the title ~~[llvm] Add support for DFP IR type.~~ [llvm] Add support for decimal floating point IR types. Oct 24, 2023

zahiraam requested review from shafik, tahonermann and jcranmer-intel October 24, 2023 21:52

zahiraam changed the title ~~[llvm] Add support for decimal floating point IR types.~~ [llvm] [DFP] Add support for decimal floating point IR types. Oct 25, 2023

zahiraam marked this pull request as ready for review October 25, 2023 11:46

jcranmer-intel reviewed Oct 25, 2023

View reviewed changes

Addressed review comments.

19ba512

tahonermann reviewed Oct 26, 2023

View reviewed changes

zahiraam added 3 commits October 30, 2023 11:21

Addressed review comments.

8efbd76

Put all the Decimal* code together.

29c7d68

Put all the Decimal* code together and removed unrelated

f07241f

changes.

jcranmer-intel reviewed Oct 31, 2023

View reviewed changes

Fixed comment.

e32b9d1

tahonermann reviewed Nov 1, 2023

View reviewed changes

Addressed review comments.

0461fcd

zahiraam requested a review from tahonermann November 2, 2023 13:32

tahonermann requested review from hubert-reinterpretcast and Ariel-Burton November 7, 2023 15:11

tahonermann mentioned this pull request Nov 7, 2023

Audit uses of llvm::type::isFloatingPointTy() and change to isBinaryFloatingPointTy() or isDecimalFloatingPointTy() as appropriate #34

Open

tahonermann reviewed Nov 7, 2023

View reviewed changes

tahonermann mentioned this pull request Nov 7, 2023

Add LLVM support for reading and writing DFP constant values in LLVM IR text #35

Open

Addressed review comments.

43e2ff9

tahonermann mentioned this pull request Nov 7, 2023

Reflect DFP encoding (BID/DPD) in llvm::DataLayout and the target datalayout directive #36

Open

zahiraam requested a review from tahonermann November 8, 2023 14:05

Ariel-Burton reviewed Nov 9, 2023

View reviewed changes

Addressed review comments.

0ba98d8

tahonermann approved these changes Nov 14, 2023

View reviewed changes

shafik reviewed Nov 14, 2023

View reviewed changes

shafik approved these changes Nov 14, 2023

View reviewed changes

zahiraam added 2 commits November 14, 2023 14:26

Fixed reference.

ff4a541

Fixed format.

6a80edd

zahiraam merged commit 738500f into dfp Nov 14, 2023

zahiraam deleted the DFPIRTypes branch December 6, 2023 14:34

		// See comment in the declaration of Type::getFPMantissaWidth();
		// it returns -1 if the FP type does not have a stable mantissa.

		/// Return the width of the mantissa of a decimal floating-point type.
		int getDFPMantissaWidth() const;

		/// Return the width of the mantissa of a decimal floating-point type, in
		/// digits. See table in section X.2.1 of WG14 N2601.

-  /// Return the width of the mantissa of a decimal floating-point type, in
-  /// digits. See table in section X.2.1 of WG14 N2601.
+  /// Return the precision in digits of a decimal floating-point type
+  /// per the "Decimal interchange format parameters" table of
+  /// C23 annex H.2.1, "Interchange floating types".

		// FIXME: Need to add code here for DFP, using the APDecimalFloat.

	// Precision values following the table in section X.2.1 of WG14 N2601.
	// Precision values following the "Decimal interchange format parameters"
	/// table of C23 annex H.2.1, "Interchange floating types".

	assert("unknown decimal floating point type");
	report_fatal_error("unknown decimal floating point type");

		case Decimal32TyID: return getDecimal32Ty(C);
		case Decimal64TyID: return getDecimal64Ty(C);

[llvm] [DFP] Add support for decimal floating point IR types. #33

[llvm] [DFP] Add support for decimal floating point IR types. #33

Uh oh!

Conversation

zahiraam commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcranmer-intel 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!

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!

jcranmer-intel commented Oct 25, 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!

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 commented Oct 26, 2023

Uh oh!

zahiraam commented Oct 26, 2023

Uh oh!

tahonermann commented Oct 26, 2023

Uh oh!

tahonermann commented Oct 26, 2023

Uh oh!

jcranmer-intel commented Oct 26, 2023

Uh oh!

tahonermann commented Oct 26, 2023

Uh oh!

tahonermann commented Oct 26, 2023

Uh oh!

jcranmer-intel commented Oct 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

zahiraam commented Oct 24, 2023 •

edited

Loading