Add support for -mdecimal-float-abi option. #43

zahiraam · 2024-08-12T19:47:40Z

This patch implements the support of -mdecimal-float-abi It sets the decimal encoding for decimal floating-point operations used.
Its syntax is:

-mdecimal-float-abi: [hard|libgcc:bid|libgcc:dpd]

Hard: use target specific encoding.
Libgcc:bid: use libgcc withBID encoding.
Libgcc:dpd: use libgcc with DPD encoding.

tahonermann

I added a number of comments. I think there are two main concerns to be addressed:

Integration with the existing DecimalFloatEnablementAndMode data member of TargetInfo.
How to make this not be so X86 specific. I think we should add the needed infrastructure to make it simple for other targets to opt-in to a default DFP ABI and to validate an explicitly provided one.

I'm also not sure I see how this ends up connecting with the proposed changes for llvm::DataLayout in #40. The option should influence the behavior of that code.

tahonermann · 2024-08-13T15:41:48Z

clang/include/clang/Basic/DiagnosticDriverKinds.td

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


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'">;

tahonermann · 2024-08-13T15:44:12Z

clang/include/clang/Basic/DiagnosticDriverKinds.td

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


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">;

tahonermann · 2024-08-13T15:50:31Z

clang/include/clang/Basic/TargetInfo.h

+  enum DecimalFloat { Libgcc_BID, Libgcc_DPD, Hard } DecimalFloatABI;
+


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.

tahonermann · 2024-08-13T20:25:46Z

clang/include/clang/Driver/Options.td

@@ -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">,


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?

tahonermann · 2024-08-13T20:30:56Z

clang/lib/Basic/Targets/X86.h

@@ -165,6 +165,7 @@ class LLVM_LIBRARY_VISIBILITY X86TargetInfo : public TargetInfo {
  bool HasUINTR = false;
  bool HasCRC32 = false;
  bool HasX87 = false;
+  bool HasBIDENCODING = false;


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.

clang/lib/CodeGen/BackendUtil.cpp

tahonermann · 2024-08-13T20:48:25Z

llvm/include/llvm/Target/TargetOptions.h

+   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.
+  };


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.

};

tahonermann · 2024-08-13T21:04:16Z

clang/lib/Driver/ToolChains/Arch/X86.cpp

+    }
+  }
+  return ABI;
+}


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.

tahonermann · 2024-08-13T21:06:54Z

clang/lib/Driver/ToolChains/Arch/X86.h

+  Libgcc_BID, // libgcc with BID encoding.
+  Libgcc_DPD, // libgcc with DPD encoding.
+  Hard,       // Target-specific hard ABI.
+  Invalid


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.

tahonermann · 2024-08-13T21:16:21Z

llvm/lib/Target/X86/X86.td

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


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".

zahiraam · 2024-08-14T15:26:49Z

I added a number of comments. I think there are two main concerns to be addressed:

Integration with the existing DecimalFloatEnablementAndMode data member of TargetInfo.

How to make this not be so X86 specific. I think we should add the needed infrastructure to make it simple for other targets to opt-in to a default DFP ABI and to validate an explicitly provided one.

I'm also not sure I see how this ends up connecting with the proposed changes for llvm::DataLayout in #40. The option should influence the behavior of that code.

Thanks for the review.

This one is tricky and still looking at it.
Yes, I agree it's x86 centric. I am looking at how to "lift" this up so that the same code applies to other targets.

As for the llvm::DataLayout in X86_64TargetInfo, we can visit the vector Opts.FeaturesAsWritten which at this point will have the +bid-encoding added if we used the mdecial-float-abi= option and set the data layout accordingly.

comments.

zahiraam · 2024-08-21T18:22:32Z

@tahonermann I have tried to respond to all your comments (some might be missing). I have made the changes so the encoding setting is target agnostic (only added X86 setting for now).
I am still looking at the issue 1. mentioned above.

tahonermann

Comments added. The structure of the changes looks good; it looks like you identified the "right" way to propagate the options through the various layers; that is where I was so stuck.

I think the most significant concern I have has to do with how the default ABI choice is made. I'm happy to talk through options for how to do that. It isn't at all obvious to me what the best approach is.

clang/include/clang/Driver/Options.td

tahonermann · 2024-09-20T15:41:27Z

clang/lib/Driver/ToolChain.cpp

+  tools::DecimalFloatABI ABI =
+      tools::getDecimalFloatABI(getDriver(), Triple, Args);
+  tools::DecimalFloatABI DefaultDecimalABI =
+      tools::getDefaultDecimalFloatABI(Triple);
+  if (DefaultDecimalABI != tools::DecimalFloatABI::None &&
+      ABI != DefaultDecimalABI) {
+    Arg *ABIArg =
+        Args.getLastArg(options::OPT_mdecimal_float_abi_EQ);
+    assert(ABIArg && "Non-default decimal float abi expected to be from arg");
+    D.Diag(diag::err_drv_unsupported_opt_for_target)
+        << ABIArg->getAsString(Args) << Triple.getTriple();
+  }


Despite the name, this function doesn't seem to actually "set" anything; it only initializes local variables. If it is only intended to validate a supported DFP ABI, then perhaps it should be named checkDecimalFloatABI(). Otherwise, perhaps there should be a FIXME or TODO comment?

Am I correct in assuming the check and diagnostic are sort of placeholder checks for now since they reject any options that are not "none" or the target default? If so, I suggest adding an option to delegate that check to the target (via a virtual function call) with a default base class implementation that does the check above. Alternatively, each target could declare which ABI modes it supports and this function could compare with that list.

Not really sure I understand your suggestion here. Are you proposing that a virtual function be added to TargetInfo? From here I don't have any way of getting to the TargetInfo. The decimal floating-point ABI needs to be computed here so that I can fill up the cc1 option. Unless I am not seeing things clearly?

clang/lib/Driver/ToolChains/Arch/X86.h

clang/lib/Driver/ToolChains/CommonArgs.cpp

clang/lib/Driver/ToolChains/Clang.cpp

clang/lib/Frontend/CompilerInvocation.cpp

clang/lib/Sema/SemaType.cpp

clang/test/Driver/decimal-float-abi.c

clang/lib/Driver/ToolChains/Arch/X86.h

llvm/lib/Target/X86/X86TargetMachine.cpp

Add support for -mdecimal-float-abi option.

c54e59f

zahiraam requested review from tahonermann, jcranmer-intel and shafik August 12, 2024 19:50

tahonermann requested changes Aug 13, 2024

View reviewed changes

Made the encoding setting target agnostic and responded to review

9f28449

comments.

tahonermann requested changes Sep 20, 2024

View reviewed changes

Addressed review comments.

718678d

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

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

		enum DecimalFloat { Libgcc_BID, Libgcc_DPD, Hard } DecimalFloatABI;

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

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

Add support for -mdecimal-float-abi option. #43

Are you sure you want to change the base?

Add support for -mdecimal-float-abi option. #43

Uh oh!

Conversation

zahiraam commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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 Aug 14, 2024

Uh oh!

zahiraam commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tahonermann left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zahiraam commented Aug 12, 2024 •

edited

Loading

zahiraam commented Aug 21, 2024 •

edited

Loading