Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
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/CodeGenOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,9 @@ class CodeGenOptions : public CodeGenOptionsBase {
/// The ABI to use for passing floating point arguments.
std::string FloatABI;

/// The encoding used for decimal floating point arguments.
std::string DecimalFloatABI;

/// The file to use for dumping bug report by `Debugify` for original
/// debug info.
std::string DIBugsReportFilePath;
Expand Down
4 changes: 4 additions & 0 deletions clang/include/clang/Basic/DiagnosticDriverKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ def err_drv_no_cuda_libdevice : Error<
"cannot find libdevice for %0; provide path to different CUDA installation "
"via '--cuda-path', or pass '-nocudalib' to build without linking with "
"libdevice">;
def err_drv_unsupported_decimal_fp_encoding_for_target : Error<
"unsupported decimal floating-point encoding '%0' for target '%1'">;
Copy link
Owner

Choose a reason for hiding this comment

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

The option value specifies both the ABI (hardware, libgcc, compiler-rt, etc...) and the encoding. Perhaps:

Suggested change
def err_drv_unsupported_decimal_fp_encoding_for_target : Error<
"unsupported decimal floating-point encoding '%0' for target '%1'">;
def err_drv_unsupported_decimal_fp_option_for_target : Error<
"unsupported decimal floating-point option '%0' for target '%1'">;


def err_drv_no_rocm_device_lib : Error<
"cannot find ROCm device library%select{| for %1|for ABI version %1}0; provide its path via "
Expand Down Expand Up @@ -242,6 +244,8 @@ def err_drv_cannot_read_config_file : Error<
"cannot read configuration file '%0': %1">;
def err_drv_arg_requires_bitcode_input: Error<
"option '%0' requires input to be LLVM bitcode">;
def err_drv_invalid_mdecimal_float_abi_EQ: Error<
"invalid argument '%0' to -mdecimal-float-abi=; each element must be one of: %1">;
Copy link
Owner

Choose a reason for hiding this comment

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

This looks to have been copied from err_drv_invalid_malign_branch_EQ (which is good), but the "each element" wording doesn't seem right. The following suggestion follows the wording for err_drv_loongarch_invalid_mfpu_EQ.

Suggested change
def err_drv_invalid_mdecimal_float_abi_EQ: Error<
"invalid argument '%0' to -mdecimal-float-abi=; each element must be one of: %1">;
def err_drv_invalid_mdecimal_float_abi_EQ: Error<
"invalid argument '%0' to -mdecimal-float-abi=; must be one of: %1">;


def err_target_unsupported_arch
: Error<"the target architecture '%0' is not supported by the target '%1'">;
Expand Down
2 changes: 2 additions & 0 deletions clang/include/clang/Basic/TargetInfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ struct TransferrableTargetInfo {
unsigned char DecimalFloat64Width, DecimalFloat64Align;
unsigned char DecimalFloat128Width, DecimalFloat128Align;

enum DecimalFloat { Libgcc_BID, Libgcc_DPD, Hard } DecimalFloatABI;

Copy link
Owner

Choose a reason for hiding this comment

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

Does the front end need to be aware of the ABI? The encoding is already exposed via DecimalFloatEnablementAndMode and the corresponding hasDecimalFloatingPoint() and getDecimalFloatingPointMode() member functions. In my prototype code, I had added a setDecimalFloatingPointMode() member function with the intent that driver code call that based on target ABI options.

// Fixed point bit widths
unsigned char ShortAccumWidth, ShortAccumAlign;
unsigned char AccumWidth, AccumAlign;
Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/Driver/Options.td
Original file line number Diff line number Diff line change
Expand Up @@ -4284,6 +4284,11 @@ def malign_double : Flag<["-"], "malign-double">, Group<m_Group>,
MarshallingInfoFlag<LangOpts<"AlignDouble">>;
let Flags = [TargetSpecific] in {
def mfloat_abi_EQ : Joined<["-"], "mfloat-abi=">, Group<m_Group>, Values<"soft,softfp,hard">;
def mdecimal_float_abi_EQ : Joined<["-"], "mdecimal-float-abi=">, Group<m_Group>,
Visibility<[ClangOption, CC1Option]>,
Values<"libgcc:bid,libgcc:dpd,hard">,
Copy link
Owner

Choose a reason for hiding this comment

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

Here will be additional libraries to be added later. I suggest putting hard at the beginning of the list so that the library options end up being colocated at the end of the list.

Suggested change
Values<"libgcc:bid,libgcc:dpd,hard">,
Values<"hard,libgcc:bid,libgcc:dpd">,

Perhaps we should allow default as an explicit opt-in to the target dependent default behavior?

HelpText<"The decimal float encoding to use">,
MarshallingInfoString<CodeGenOpts<"DecimalFloatABI">>;
def mfpmath_EQ : Joined<["-"], "mfpmath=">, Group<m_Group>;
def mfpu_EQ : Joined<["-"], "mfpu=">, Group<m_Group>;
def mhwdiv_EQ : Joined<["-"], "mhwdiv=">, Group<m_Group>;
Expand Down
3 changes: 2 additions & 1 deletion clang/lib/Basic/Targets/X86.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,8 @@ bool X86TargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
HasX87 = true;
} else if (Feature == "+fullbf16") {
HasFullBFloat16 = true;
}
} else if (Feature == "+bid-encoding")
HasBIDENCODING = true;
Copy link
Owner

Choose a reason for hiding this comment

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

What is the motivation for using target dependent features? Neither BID nor DPD are target dependent though the default selection and availability of a particular encoding may be. I'm inclined more towards the use of target independent options with target dependent validation.


X86SSEEnum Level = llvm::StringSwitch<X86SSEEnum>(Feature)
.Case("+avx512f", AVX512F)
Expand Down
1 change: 1 addition & 0 deletions clang/lib/Basic/Targets/X86.h
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ class LLVM_LIBRARY_VISIBILITY X86TargetInfo : public TargetInfo {
bool HasUINTR = false;
bool HasCRC32 = false;
bool HasX87 = false;
bool HasBIDENCODING = false;
Copy link
Owner

Choose a reason for hiding this comment

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

Encoding information can already be recorded in the DecimalFloatEnablementAndMode data member of TargetInfo. We shouldn't need to store this information on a target specific basis.


protected:
llvm::X86::CPUKind CPU = llvm::X86::CK_None;
Expand Down
6 changes: 6 additions & 0 deletions clang/lib/CodeGen/BackendUtil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,12 @@ static bool initTargetOptions(DiagnosticsEngine &Diags,
.Case("softfp", llvm::FloatABI::Soft)
.Case("hard", llvm::FloatABI::Hard)
.Default(llvm::FloatABI::Default);
Options.DecimalFloatABIType =
llvm::StringSwitch<llvm::DecimalFloatABI>(CodeGenOpts.DecimalFloatABI)
.Case("libgcc:bid", llvm::DecimalFloatABI::Libgcc_BID)
.Case("libgcc:dpd", llvm::DecimalFloatABI::Libgcc_DPD)
.Case("hard", llvm::DecimalFloatABI::Hard)
.Default(llvm::DecimalFloatABI::Default);

// Set FP fusion mode.
switch (LangOpts.getDefaultFPContractMode()) {
Expand Down
1 change: 1 addition & 0 deletions clang/lib/Driver/ToolChain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "clang/Driver/ToolChain.h"
#include "ToolChains/Arch/AArch64.h"
#include "ToolChains/Arch/ARM.h"
#include "ToolChains/Arch/X86.h"
#include "ToolChains/Clang.h"
#include "ToolChains/CommonArgs.h"
#include "ToolChains/Flang.h"
Expand Down
40 changes: 40 additions & 0 deletions clang/lib/Driver/ToolChains/Arch/X86.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -273,4 +273,44 @@ void x86::getX86TargetFeatures(const Driver &D, const llvm::Triple &Triple,
Features.push_back("+prefer-no-gather");
if (Args.hasArg(options::OPT_mno_scatter))
Features.push_back("+prefer-no-scatter");

// -mdecimal-float-abi
x86::DecimalFloatABI ABI = x86::getX86DecimalFloatABI(D, Triple, Args);
if (ABI == DecimalFloatABI::Invalid)
ABI = x86::getDefaultX86DecimalFloatABI(Triple, Args);
if (ABI != x86::DecimalFloatABI::None && ABI != x86::DecimalFloatABI::Default)
Features.push_back("+bid-encoding");
}

x86::DecimalFloatABI
x86::getDefaultX86DecimalFloatABI(const llvm::Triple &Triple,
const ArgList &Args) {
if (const Arg *A = Args.getLastArg(options::OPT_fdecimal_floating_point))
return DecimalFloatABI::Libgcc_BID;
else
return DecimalFloatABI::None;
}

// Get the decimal float ABI type from the command line arguments
// -mdecimal-float-abi=.
x86::DecimalFloatABI x86::getX86DecimalFloatABI(const Driver &D,
const llvm::Triple &Triple,
const ArgList &Args) {
x86::DecimalFloatABI ABI = x86::DecimalFloatABI::Invalid;
if (const Arg *A = Args.getLastArg(options::OPT_mdecimal_float_abi_EQ)) {
StringRef Val = A->getValue();
ABI = llvm::StringSwitch<x86::DecimalFloatABI>(A->getValue())
.Case("libgcc:bid", DecimalFloatABI::Libgcc_BID)
.Case("libgcc:dpd", DecimalFloatABI::Libgcc_DPD)
.Case("hard", DecimalFloatABI::Hard)
.Default(DecimalFloatABI::None);
if (ABI != x86::DecimalFloatABI::None &&
ABI != x86::DecimalFloatABI::Default) {
// X86 targets only allow BID encoding.
if (ABI != x86::DecimalFloatABI::Libgcc_BID)
D.Diag(diag::err_drv_unsupported_decimal_fp_encoding_for_target)
<< Val << "X86";
}
}
return ABI;
}
Copy link
Owner

Choose a reason for hiding this comment

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

I'd like to see the querying of the ArgList moved to a target independent location. The parsed value (one of the DecimalFloatABI enumerators) can then be passed to a target dependent interface (e.g., a virtual function on TargetInfo or perhaps ToolChain) for validation.

16 changes: 16 additions & 0 deletions clang/lib/Driver/ToolChains/Arch/X86.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,29 @@ namespace driver {
namespace tools {
namespace x86 {

enum class DecimalFloatABI {
Default, // Target-specific default.
None, // No decimal floating-point support.
Libgcc_BID, // libgcc with BID encoding.
Libgcc_DPD, // libgcc with DPD encoding.
Hard, // Target-specific hard ABI.
Invalid
Copy link
Owner

Choose a reason for hiding this comment

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

What is the motivation for an Invalid enumerator? A comment regarding its intended purpose would be helpful. My intuition is that an invalid ABI (whether an unrecognized argument provided with -mdecimal-float-abi or a unsupported ABI for a given target) would be diagnosed and the compiler invocation aborted.

};

std::string getX86TargetCPU(const Driver &D, const llvm::opt::ArgList &Args,
const llvm::Triple &Triple);

void getX86TargetFeatures(const Driver &D, const llvm::Triple &Triple,
const llvm::opt::ArgList &Args,
std::vector<llvm::StringRef> &Features);

DecimalFloatABI getX86DecimalFloatABI(const Driver &D,
const llvm::Triple &Triple,
const llvm::opt::ArgList &Args);

DecimalFloatABI getDefaultX86DecimalFloatABI(const llvm::Triple &Triple,
const llvm::opt::ArgList &Args);

} // end namespace x86
} // end namespace target
} // end namespace driver
Expand Down
10 changes: 10 additions & 0 deletions clang/lib/Driver/ToolChains/Clang.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2818,6 +2818,16 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
switch (optID) {
default:
break;
case options::OPT_mdecimal_float_abi_EQ: {
StringRef Val = A->getValue();
if (Val == "libgcc:bid" || Val == "libgcc:dpd" || Val == "hard") {
CmdArgs.push_back(Args.MakeArgString("-mdecimal-float-abi=" + Val));
} else {
D.Diag(diag::err_drv_unsupported_option_argument)
<< A->getSpelling() << Val;
}
break;
}
case options::OPT_ffp_model_EQ: {
// If -ffp-model= is seen, reset to fno-fast-math
HonorINFs = true;
Expand Down
28 changes: 28 additions & 0 deletions clang/test/Driver/decimal-float-abi.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// DEFINE: %{common_opts_x86} = -### \
// DEFINE: -fexperimental-decimal-floating-point

// X86
// RUN: %clang %{common_opts_x86} --target=x86_64 \
// RUN: -mdecimal-float-abi=libgcc:bid %s 2>&1 | FileCheck -check-prefix=BID %s

// RUN: not %clang %{common_opts_x86} --target=x86_64 \
// RUN: -mdecimal-float-abi=libgcc:dpd %s 2>&1 \
// RUN: | FileCheck -check-prefix=ERROR_DPD %s

// RUN: not %clang %{common_opts_x86} --target=x86_64 \
// RUN: -mdecimal-float-abi=hard %s 2>&1 \
// RUN: | FileCheck -check-prefix=ERROR_HARD %s

// RUN: not %clang %{common_opts_x86} --target=mips64-unknown-linux \
// RUN: -mdecimal-float-abi=hard %s 2>&1 \
// RUN: | FileCheck -check-prefix=ERROR_MIPS_TRGT %s

// BID: "-mdecimal-float-abi=libgcc:bid" {{.*}} "-target-feature" "+bid-encoding"

// ERROR_DPD: unsupported decimal floating-point encoding 'libgcc:dpd' for target 'X86'
// ERROR_HARD: unsupported decimal floating-point encoding 'hard' for target 'X86'
// ERROR_MIPS_TRGT: unsupported option '-mdecimal-float-abi=' for target 'mips64-unknown-linux'

// PPC
// S390
// ARM
2 changes: 2 additions & 0 deletions llvm/include/llvm/CodeGen/CommandFlags.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ bool getEnableHonorSignDependentRoundingFPMath();

llvm::FloatABI::ABIType getFloatABIForCalls();

llvm::DecimalFloatABI getDecimalFloatABIForCalls();

llvm::FPOpFusion::FPOpFusionMode getFuseFPOps();

SwiftAsyncFramePointerMode getSwiftAsyncFramePointer();
Expand Down
10 changes: 10 additions & 0 deletions llvm/include/llvm/Target/TargetOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,14 @@ namespace llvm {
};
}

enum class DecimalFloatABI {
Default, // Target-specific.
None, // No decimal floating-point support.
Libgcc_BID, // libgcc with BID encoding.
Libgcc_DPD, // libgcc with DPD encoding.
Hard // Hardware with target-specific encoding.
};
Copy link
Owner

Choose a reason for hiding this comment

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

There is an extra space character before the enum declaration. Per other comments, I'd like to see the library options at the end of the list.

Suggested change
enum class DecimalFloatABI {
Default, // Target-specific.
None, // No decimal floating-point support.
Libgcc_BID, // libgcc with BID encoding.
Libgcc_DPD, // libgcc with DPD encoding.
Hard // Hardware with target-specific encoding.
};
enum class DecimalFloatABI {
Default, // Target-specific.
None, // No decimal floating-point support.
Hard, // Hardware with target-specific encoding.
Libgcc_BID, // libgcc with BID encoding.
Libgcc_DPD // libgcc with DPD encoding.
};


namespace FPOpFusion {
enum FPOpFusionMode {
Fast, // Enable fusion of FP ops wherever it's profitable.
Expand Down Expand Up @@ -376,6 +384,8 @@ namespace llvm {
/// arm-apple-darwin). Hard presumes that the normal FP ABI is used.
FloatABI::ABIType FloatABIType = FloatABI::Default;

DecimalFloatABI DecimalFloatABIType = DecimalFloatABI::Default;

/// AllowFPOpFusion - This flag is set by the -fp-contract=xxx option.
/// This controls the creation of fused FP ops that store intermediate
/// results in higher precision than IEEE allows (E.g. FMAs).
Expand Down
3 changes: 3 additions & 0 deletions llvm/lib/Target/X86/X86.td
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ def FeatureXSAVES : SubtargetFeature<"xsaves", "HasXSAVES", "true",
"Support xsaves instructions",
[FeatureXSAVE]>;

def FeatureBIDENCODING : SubtargetFeature<"bid-encoding", "HasBIDENCODING", "true",
"Support BID encoding for decimal floatin-points">;
Copy link
Owner

Choose a reason for hiding this comment

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

Per other comments, I think we need to figure out how to make this not be X86 specific.

Also, typo: "decimal floatin-points" -> "decimal floating-point".


def FeatureSSE1 : SubtargetFeature<"sse", "X86SSELevel", "SSE1",
"Enable SSE instructions">;
def FeatureSSE2 : SubtargetFeature<"sse2", "X86SSELevel", "SSE2",
Expand Down
1 change: 1 addition & 0 deletions llvm/lib/Target/X86/X86InstrInfo.td
Original file line number Diff line number Diff line change
Expand Up @@ -929,6 +929,7 @@ def HasAVXVNNIINT16 : Predicate<"Subtarget->hasAVXVNNIINT16()">;
def HasAVXVNNIINT8 : Predicate<"Subtarget->hasAVXVNNIINT8()">;
def HasAVXVNNI : Predicate <"Subtarget->hasAVXVNNI()">;
def NoVLX_Or_NoVNNI : Predicate<"!Subtarget->hasVLX() || !Subtarget->hasVNNI()">;
def HasBIDENCODING : Predicate<"Subtarget->hasBIDENCODING()">;

def HasBITALG : Predicate<"Subtarget->hasBITALG()">;
def HasPOPCNT : Predicate<"Subtarget->hasPOPCNT()">;
Expand Down
9 changes: 9 additions & 0 deletions llvm/lib/Target/X86/X86TargetMachine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,15 @@ X86TargetMachine::X86TargetMachine(const Target &T, const Triple &TT,
this->Options.NoTrapAfterNoreturn = TT.isOSBinFormatMachO();
}

// Default to triple-appropriate decimal float ABI.
if (Options.DecimalFloatABIType == DecimalFloatABI::None) {
this->Options.DecimalFloatABIType = DecimalFloatABI::None;
} else {
if (Options.DecimalFloatABIType == DecimalFloatABI::Default) {
this->Options.DecimalFloatABIType = DecimalFloatABI::Libgcc_BID;
}
}

setMachineOutliner(true);

// x86 supports the debug entry values.
Expand Down