Skip to content

[llvm][DFP] Add support for DFP encoding in llvm::DataLayout. #40

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 6 commits into
base: dfp
Choose a base branch
from
Draft

[llvm][DFP] Add support for DFP encoding in llvm::DataLayout. #40

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7b404ff
Add support for DFP encoding in llvm::DataLayout.
zahiraam Dec 6, 2023
60a6a1c
Added LIT test.
zahiraam Dec 6, 2023
e4f4016
Code cleanup.
zahiraam Dec 6, 2023
ee642f5
More cleanup.
zahiraam Dec 6, 2023
69e623b
Addressed review comments.
zahiraam Jan 29, 2024
e92106f
Code cleanup.
zahiraam Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/include/clang/Basic/TargetOptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ class TargetOptions {
/// \brief Code object version for AMDGPU.
CodeObjectVersionKind CodeObjectVersion = CodeObjectVersionKind::COV_None;

enum class BinaryEncoding { BID, DPD, None };
BinaryEncoding DFPEncoding = BinaryEncoding::None;
Copy link
Owner

Choose a reason for hiding this comment

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

We already have something similar in the TargetInfo.h header as shown below. I still haven't been able to get a good understanding for when TargetInfo is used vs TargetOptions. Can you look to see if it makes sense to use the data already present in TargetInfo?

enum class DecimalFloatMode : uint8_t {
  BID,  // Binary Integer Decimal.
  DPD,  // Densely Packed Decimal.
};
...
class TargetInfo : public TransferrableTargetInfo,
                   public RefCountedBase<TargetInfo> {
  ...
  // If disengaged, decimal floating-point extensions are not supported,
  // otherwise, the decimal floating-point mode that is enabled.
  std::optional<DecimalFloatMode> DecimalFloatEnablementAndMode;
  ...
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mentioned this in the chat earlier today, but it should probably go in include/llvm/ADT/FloatingPointMode.h, which contains the other FP env modes.

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you, yes, that looks exactly right.

Copy link
Collaborator Author

@zahiraam zahiraam Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Moved the enum to FloatingPointMode.h. Tried to rest into TargetInfo but that doesn't seem to work. It needs to be in clang::TargetOptions. The reason being that in initTargetOptions we are passing the info from clang::TargetOptions to llvm::TargetOptions and there is no access to TargetInfo here. So the info about the encoding needs to reside in targetOptions. Will keep looking at it.


/// \brief Enumeration values for AMDGPU printf lowering scheme
enum class AMDGPUPrintfKind {
/// printf lowering scheme involving hostcalls, currently used by HIP
Expand Down
32 changes: 26 additions & 6 deletions clang/lib/Basic/Targets/X86.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,13 +719,33 @@ class LLVM_LIBRARY_VISIBILITY X86_64TargetInfo : public X86TargetInfo {
Int64Type = IsX32 ? SignedLongLong : SignedLong;
RegParmMax = 6;

bool isBIDEncoding = Opts.DFPEncoding == TargetOptions::BinaryEncoding::BID;
bool isDPDEncoding = Opts.DFPEncoding == TargetOptions::BinaryEncoding::DPD;
Copy link
Owner

Choose a reason for hiding this comment

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

At present, this information should already be available on the TargetInfo base class via the DecimalFloatEnablementAndMode member at this point (if hasDecimalFloatingPoint() returns true, then getDecimalFloatingPointMode() will return the active DFP mode).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See comment above.


// Pointers are 32-bit in x32.
resetDataLayout(IsX32 ? "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-"
"i64:64-f80:128-n8:16:32:64-S128"
: IsWinCOFF ? "e-m:w-p270:32:32-p271:32:32-p272:64:"
"64-i64:64-f80:128-n8:16:32:64-S128"
: "e-m:e-p270:32:32-p271:32:32-p272:64:"
"64-i64:64-f80:128-n8:16:32:64-S128");
if (isBIDEncoding)
resetDataLayout(
IsX32 ? "e-d:-bid-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-"
"i64:64-f80:128-n8:16:32:64-S128"
: IsWinCOFF ? "e-d:bid-m:w-p270:32:32-p271:32:32-p272:64:"
"64-i64:64-f80:128-n8:16:32:64-S128"
: "e-d:bid-m:e-p270:32:32-p271:32:32-p272:64:"
"64-i64:64-f80:128-n8:16:32:64-S128");
else if (isDPDEncoding)
resetDataLayout(
IsX32 ? "e-d:dpd-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-"
"i64:64-f80:128-n8:16:32:64-S128"
: IsWinCOFF ? "e-d:dpd-m:w-p270:32:32-p271:32:32-p272:64:"
"64-i64:64-f80:128-n8:16:32:64-S128"
: "e-d:dpd-m:e-p270:32:32-p271:32:32-p272:64:"
"64-i64:64-f80:128-n8:16:32:64-S128");
else
resetDataLayout(IsX32 ? "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-"
"i64:64-f80:128-n8:16:32:64-S128"
: IsWinCOFF ? "e-m:w-p270:32:32-p271:32:32-p272:64:"
"64-i64:64-f80:128-n8:16:32:64-S128"
: "e-m:e-p270:32:32-p271:32:32-p272:64:"
"64-i64:64-f80:128-n8:16:32:64-S128");
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than duplicating the existing strings, I think this could be simplified with some string building. See the WindowsX86_32TargetInfo constructor later in this same file for an example.


// Use fpret only for long double.
RealTypeUsesObjCFPRetMask = (unsigned)FloatModeKind::LongDouble;
Expand Down
7 changes: 7 additions & 0 deletions clang/lib/CodeGen/BackendUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,13 @@ static bool initTargetOptions(DiagnosticsEngine &Diags,
break;
}

if (TargetOpts.DFPEncoding == clang::TargetOptions::BinaryEncoding::BID)
Options.DFPEncoding = llvm::BinaryEncoding::BID;
else if (TargetOpts.DFPEncoding == clang::TargetOptions::BinaryEncoding::DPD)
Options.DFPEncoding = llvm::BinaryEncoding::DPD;
else
Options.DFPEncoding = llvm::BinaryEncoding::None;

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, this suggests that the enumeration that is currently defined in clang/include/clang/Basic/TargetInfo.h should get moved to llvm/include/llvm/Target/TargetOptions.h. Or perhaps should be incorporated into APFloatBase in llvm/include/llvm/ADT/APFloat.h. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly what I have done :-) Not sure about APFloat.h. I will take a look. For now it's residing here.

Options.BinutilsVersion =
llvm::TargetMachine::parseBinutilsVersion(CodeGenOpts.BinutilsVersion);
Options.UseInitArray = CodeGenOpts.UseInitArray;
Expand Down
9 changes: 9 additions & 0 deletions clang/lib/Frontend/CompilerInstance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,15 @@ void CompilerInstance::setTarget(TargetInfo *Value) { Target = Value; }
void CompilerInstance::setAuxTarget(TargetInfo *Value) { AuxTarget = Value; }

bool CompilerInstance::createTarget() {
if (getLangOpts().DecimalFloatingPoint) {
if (getInvocation().getTargetOpts().CPU == "x86" ||
getInvocation().getTargetOpts().CPU == "x86-64")
getInvocation().getTargetOpts().DFPEncoding =
TargetOptions::BinaryEncoding::BID;
else
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know that this logic is correct for most targets.

libdfp only mentions x86(-64), PPC and s390 in its documentation (see https://github.com/libdfp/libdfp), but helpfully, the most recent commit suggests that aarch64 is going to go with BID.

The AArch64 suggests that it's using BID: https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#decimal-floating-point (there's nothing corresponding I could find for 32-bit arm).

Copy link
Owner

Choose a reason for hiding this comment

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

It isn't right. I suspect Zahira derived this from similar code I added to the LinuxTargetInfo constructor in clang/lib/Basic/Targets/OSTargets.h. I'm still working on code to enable proper target dependent enablement. I've been taking forever to get that done.

I think our plan for prior to upstreaming should be to focus on just Intel support (perhaps IBM too given Ariel's involvement) and then let maintainers for other architectures enable support for other targets following appropriate testing after our changes are accepted upstream.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Again the change you are suggesting implies that the encoding info is in TargetInfo but I don't think we can do that there. See comments above.

getInvocation().getTargetOpts().DFPEncoding =
TargetOptions::BinaryEncoding::DPD;
}
Copy link
Owner

Choose a reason for hiding this comment

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

This should be addressed by the code changes I've been taking forever to finish. There is already code that does similarly to populate TargetInfo::DecimalFloatEnablementAndMode; see the assignment to DecimalFloatEnablementAndMode in clang/lib/Basic/Targets/OSTargets.h.

// Create the target instance.
setTarget(TargetInfo::CreateTargetInfo(getDiagnostics(),
getInvocation().TargetOpts));
Expand Down
8 changes: 8 additions & 0 deletions clang/test/CodeGen/dfp-dl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// RUN: %clang_cc1 -emit-llvm -triple x86_64 -target-cpu x86-64 -emit-llvm -o - %s | FileCheck %s

// RUN: %clang_cc1 -emit-llvm -triple x86_64 -target-cpu x86-64 -emit-llvm -fexperimental-decimal-floating-point -o - %s | FileCheck %s --check-prefixes=DFP

// CHECK: target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
// DFP: target datalayout = "e-d:bid-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
void foo() {
}
4 changes: 4 additions & 0 deletions llvm/include/llvm/IR/DataLayout.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,9 @@ class DataLayout {
/// Defaults to false.
bool BigEndian;

enum BinaryEncoding { BID, DPD, None };
BinaryEncoding DFPEncoding;

unsigned AllocaAddrSpace;
MaybeAlign StackNaturalAlign;
unsigned ProgramAddrSpace;
Expand Down Expand Up @@ -205,6 +208,7 @@ class DataLayout {
clear();
StringRepresentation = DL.StringRepresentation;
BigEndian = DL.isBigEndian();
DFPEncoding = DL.DFPEncoding;
AllocaAddrSpace = DL.AllocaAddrSpace;
StackNaturalAlign = DL.StackNaturalAlign;
FunctionPtrAlign = DL.FunctionPtrAlign;
Expand Down
4 changes: 4 additions & 0 deletions llvm/include/llvm/Target/TargetOptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ namespace llvm {
GNU
};

enum class BinaryEncoding { BID, DPD, None };

/// Identify a debugger for "tuning" the debug info.
///
/// The "debugger tuning" concept allows us to present a more intuitive
Expand Down Expand Up @@ -404,6 +406,8 @@ namespace llvm {
/// Which debugger to tune for.
DebuggerKind DebuggerTuning = DebuggerKind::Default;

BinaryEncoding DFPEncoding = BinaryEncoding::None;

private:
/// Flushing mode to assume in default FP environment.
DenormalMode FPDenormalMode;
Expand Down
9 changes: 9 additions & 0 deletions llvm/lib/IR/DataLayout.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ void DataLayout::reset(StringRef Desc) {

LayoutMap = nullptr;
BigEndian = false;
DFPEncoding = None;
AllocaAddrSpace = 0;
StackNaturalAlign.reset();
ProgramAddrSpace = 0;
Expand Down Expand Up @@ -318,6 +319,14 @@ Error DataLayout::parseSpecifier(StringRef Desc) {
case 'e':
BigEndian = false;
break;
case 'd':
if (Split.second == "bid")
DFPEncoding = BID;
else if (Split.second == "dpd")
DFPEncoding = DPD;
else
DFPEncoding = None;
break;
case 'p': {
// Address space.
unsigned AddrSpace = 0;
Expand Down
12 changes: 10 additions & 2 deletions llvm/lib/Target/X86/X86TargetMachine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,18 @@ static std::unique_ptr<TargetLoweringObjectFile> createTLOF(const Triple &TT) {
return std::make_unique<X86ELFTargetObjectFile>();
}

static std::string computeDataLayout(const Triple &TT) {
static std::string computeDataLayout(const Triple &TT,
const TargetOptions &Options) {
// X86 is little endian
std::string Ret = "e";

// Decimal floating-point encoding.
if (Options.DFPEncoding != BinaryEncoding::None) {
assert(Options.DFPEncoding == BinaryEncoding::BID &&
"Decimal floating-point on x86 is BID!");
Ret += "-d:bid";
}

Copy link
Owner

Choose a reason for hiding this comment

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

There are multiple places where these strings have to be built for each target? That seems very unfortunate.

Ret += DataLayout::getManglingComponent(TT);
// X86 and x32 have 32 bit pointers.
if (!TT.isArch64Bit() || TT.isX32() || TT.isOSNaCl())
Expand Down Expand Up @@ -226,7 +234,7 @@ X86TargetMachine::X86TargetMachine(const Target &T, const Triple &TT,
std::optional<CodeModel::Model> CM,
CodeGenOptLevel OL, bool JIT)
: LLVMTargetMachine(
T, computeDataLayout(TT), TT, CPU, FS, Options,
T, computeDataLayout(TT, Options), TT, CPU, FS, Options,
getEffectiveRelocModel(TT, JIT, RM),
getEffectiveX86CodeModel(CM, JIT, TT.getArch() == Triple::x86_64),
OL),
Expand Down