[clang] Make -fveclib={ArmPL,SLEEF} imply -fno-math-errno #112580

MacDue · 2024-10-16T16:44:17Z

These two veclibs are only available for AArch64 targets, and as mentioned in https://discourse.llvm.org/t/rfc-should-fveclib-imply-fno-math-errno-for-all-targets/81384, we (Arm) think that -fveclib should imply -fno-math-errno. By setting -fveclib the user shows they intend to use the vector math functions, which implies they don't care about errno. However, currently, the vector mappings won't be used in many cases without setting -fno-math-errno separately.

Making this change would also help resolve some inconsistencies in how vector mappings are applied (see #108980 (comment)).

Note: Both SLEEF and ArmPL state that they do not set errno:

https://developer.arm.com/documentation/101004/2410/General-information/Arm-Performance-Libraries-math-functions
- "The vector functions in libamath which are available on Linux may not set errno nor raise exceptions"
https://sleef.org/2-references/libm/
- "These functions do not set errno nor raise an exception."

llvmbot · 2024-10-16T16:44:55Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Benjamin Maxwell (MacDue)

Changes

These two veclibs are only available for AArch64 targets, and as mentioned in https://discourse.llvm.org/t/rfc-should-fveclib-imply-fno-math-errno-for-all-targets/81384, we (Arm) think that -fveclib should imply -fno-math-errno. By setting -fveclib the user shows they intend to use the vector math functions, which implies they don't care about errno. However, currently, the vector mappings won't be used in many cases without setting -fno-math-errno separately.

Making this change would also help resolve some inconsistencies in how vector mappings are applied (see #108980 (comment)).

Note: Both SLEEF and ArmPL state that they do not set errno:

https://developer.arm.com/documentation/101004/2410/General-information/Arm-Performance-Libraries-math-functions
- "The vector functions in libamath which are available on Linux may not set errno nor raise exceptions"
https://sleef.org/2-references/libm/
- "These functions do not set errno nor raise an exception."

Full diff: https://github.com/llvm/llvm-project/pull/112580.diff

3 Files Affected:

(modified) clang/include/clang/Driver/Options.td (+2-1)
(modified) clang/lib/Driver/ToolChains/Clang.cpp (+8)
(modified) clang/test/Driver/fveclib.c (+7)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 379e75b197cf96..7965f70e290408 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -3410,7 +3410,8 @@ def fno_experimental_isel : Flag<["-"], "fno-experimental-isel">, Group<f_clang_
   Alias<fno_global_isel>;
 def fveclib : Joined<["-"], "fveclib=">, Group<f_Group>,
   Visibility<[ClangOption, CC1Option, FlangOption, FC1Option]>,
-    HelpText<"Use the given vector functions library">,
+    HelpText<"Use the given vector functions library."
+             "Note: -fveclib={ArmPL,SLEEF} implies -fno-math-errno">,
     Values<"Accelerate,libmvec,MASSV,SVML,SLEEF,Darwin_libsystem_m,ArmPL,AMDLIBM,none">,
     NormalizedValuesScope<"llvm::driver::VectorLibrary">,
     NormalizedValues<["Accelerate", "LIBMVEC", "MASSV", "SVML", "SLEEF",
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 3fc39296f44281..7e7f3770cfb62d 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -2854,6 +2854,10 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
                                        bool OFastEnabled, const ArgList &Args,
                                        ArgStringList &CmdArgs,
                                        const JobAction &JA) {
+  // List of veclibs which when used with -fveclib imply -fno-math-errno.
+  constexpr std::array VecLibImpliesNoMathErrno{llvm::StringLiteral("ArmPL"),
+                                                llvm::StringLiteral("SLEEF")};
+
   // Handle various floating point optimization flags, mapping them to the
   // appropriate LLVM code generation flags. This is complicated by several
   // "umbrella" flags, so we do this by stepping through the flags incrementally
@@ -3125,6 +3129,10 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
       TrappingMathPresent = true;
       FPExceptionBehavior = "strict";
       break;
+    case options::OPT_fveclib:
+      if (llvm::is_contained(VecLibImpliesNoMathErrno, A->getValue()))
+        MathErrno = false;
+      break;
     case options::OPT_fno_trapping_math:
       if (!TrappingMathPresent && !FPExceptionBehavior.empty() &&
           FPExceptionBehavior != "ignore")
diff --git a/clang/test/Driver/fveclib.c b/clang/test/Driver/fveclib.c
index 9b0f1ce13aa2bd..2a3133541e3b72 100644
--- a/clang/test/Driver/fveclib.c
+++ b/clang/test/Driver/fveclib.c
@@ -36,16 +36,23 @@
 /* Verify that the correct vector library is passed to LTO flags. */
 
 // RUN: %clang -### --target=x86_64-unknown-linux-gnu -fveclib=LIBMVEC -flto %s 2>&1 | FileCheck --check-prefix=CHECK-LTO-LIBMVEC %s
+// CHECK-LTO-LIBMVEC: "-fmath-errno"
 // CHECK-LTO-LIBMVEC: "-plugin-opt=-vector-library=LIBMVEC-X86"
 
 // RUN: %clang -### --target=powerpc64-unknown-linux-gnu -fveclib=MASSV -flto %s 2>&1 | FileCheck --check-prefix=CHECK-LTO-MASSV %s
+// CHECK-LTO-MASSV: "-fmath-errno"
 // CHECK-LTO-MASSV: "-plugin-opt=-vector-library=MASSV"
 
 // RUN: %clang -### --target=x86_64-unknown-linux-gnu -fveclib=SVML -flto %s 2>&1 | FileCheck --check-prefix=CHECK-LTO-SVML %s
+// CHECK-LTO-SVML: "-fmath-errno"
 // CHECK-LTO-SVML: "-plugin-opt=-vector-library=SVML"
 
 // RUN: %clang -### --target=aarch64-linux-gnu -fveclib=SLEEF -flto %s 2>&1 | FileCheck --check-prefix=CHECK-LTO-SLEEF %s
+// CHECK-LTO-SLEEF-NOT: "-fmath-errno"
 // CHECK-LTO-SLEEF: "-plugin-opt=-vector-library=sleefgnuabi"
+// CHECK-LTO-SLEEF-NOT: "-fmath-errno"
 
 // RUN: %clang -### --target=aarch64-linux-gnu -fveclib=ArmPL -flto %s 2>&1 | FileCheck --check-prefix=CHECK-LTO-ARMPL %s
+// CHECK-LTO-ARMPL-NOT: "-fmath-errno"
 // CHECK-LTO-ARMPL: "-plugin-opt=-vector-library=ArmPL"
+// CHECK-LTO-ARMPL-NOT: "-fmath-errno"

kiranchandramohan

Thanks for this patch.

Have you checked the flang driver? Is it not applicable there since errno is not used in Flang?

kiranchandramohan · 2024-10-16T18:16:26Z

clang/lib/Driver/ToolChains/Clang.cpp

+    case options::OPT_fveclib:
+      if (llvm::is_contained(VecLibImpliesNoMathErrno, A->getValue()))
+        MathErrno = false;
+      break;


Should we check specifically for the Arm platform?

I don't thinks so, these 2 libraries are stating clearly that they do not honour errno regardless of the platform. Well in ArmPL case is supports only Arm, but SLEEF could work on other targets if the right mappings were added to LLVM.

As @mgabka says, this is a property of the library rather than an architecture specific decision.

DavidTruby · 2024-10-17T05:45:33Z

Have you checked the flang driver? Is it not applicable there since errno is not used in Flang?

We don't support the gfortran extension for checking errno in flang and I can't see another way of checking it portably, so I wonder if we should just have this flag on by default in flang in general? It shouldn't provide any observable change and might increase performance as far as I can tell.

We also don't actually check errno in any of the math calls in the Fortran runtime either so we might want it on when building there as well.

The change for clang looks good to me but don't want to "approve" as this probably needs someone with more specific knowledge of the clang side of things.

mgabka · 2024-10-17T08:18:06Z

clang/test/Driver/fveclib.c


 // RUN: %clang -### --target=aarch64-linux-gnu -fveclib=ArmPL -flto %s 2>&1 | FileCheck --check-prefix=CHECK-LTO-ARMPL %s
+// CHECK-LTO-ARMPL-NOT: "-fmath-errno"


I think it might be worth to add tests for "-fveclib=ArmPL -fmath-errno" and similar for sleefm to ensure that the order of flags in properly honoured.

I agree but I think it should be stronger and we should emit a warning diagnostic that says the potential to use the specified vector library will be hampered because -fmath-errnor has been requested.

I'm suggesting a warning rather than remark because we're basically saying the user has likely made a mistake because it doesn't make sense to request both features.

I've added the extra tests for -fmath-errno after -fveclib.

If we want to add a warning, there are a few cases to consider where errno would be re-enabled:

-fveclib=ArmPL -fmath-errno (the obvious case)

-fveclib=ArmPL -fno-fast-math

-fveclib=ArmPL -ffp-model=strict|precise

Would a warning be expected for all of these?

Yes. Any scenario where MathErrno ends up being true after -fveclib=ArmPL has cleared it should generate the warning. I'm expecting this to keeps things simple because it can be checked once as part of the code that then adds the -fmath-errno option.

I think it'll also need to track which option/arg caused -fmath-errno to be set (otherwise the warning would be somewhat unhelpful in some cases).

I'm not too worried about that, it's not really the responsibility of this warning to explain why math-errno is in effect.

Ah well, I did add it to the diagnostic 😅 (I don't think it adds much complexity, and may be helpful), but I don't mind removing it 🙂

Thanks @MacDue for these changes, personally I find the warning in the current form helpful, although it feels to me like it combines 2 things:

warning that math-errno got re-enabled

loop vectorizer optimization remark suggesting to use -fno-math-errno to enable vectorization loops calling math functions.

Do we have the pt 2 already?

I've followed @paulwalker-arm's suggestion and removed the vectorizer specific phrasing from the warning, so it's now:

math errno enabled by '-ffp-model=strict' after it was implicitly disabled by '-fveclib=ArmPL', this may limit the utilization of the vector library [-Wmath-errno-enabled-with-veclib]

Which I think addresses the "combines 2 things" issue. I'm not sure if pt 2 exists. Is errno a concept that exists in the loop vectorizer, other than as a memory effect? It seems more C/C++ language specific.

That said, I did spot:

llvm-project/llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

Lines 908 to 918 in e13f1d1

if (IsMathLibCall) {

// TODO: Ideally, we should not use clang-specific language here,

// but it's hard to provide meaningful yet generic advice.

// Also, should this be guarded by allowExtraAnalysis() and/or be part

// of the returned info from isFunctionVectorizable()?

reportVectorizationFailure(

"Found a non-intrinsic callsite",

"library call cannot be vectorized. "

"Try compiling with -fno-math-errno, -ffast-math, "

"or similar flags",

"CantVectorizeLibcall", ORE, TheLoop, CI);

MacDue · 2024-10-17T08:56:24Z

Have you checked the flang driver? Is it not applicable there since errno is not used in Flang?

We don't support the gfortran extension for checking errno in flang and I can't see another way of checking it portably, so I wonder if we should just have this flag on by default in flang in general? It shouldn't provide any observable change and might increase performance as far as I can tell.

As far as I can tell from looking at the flang driver, it already defaults to the equivalent of -fno-math-errno (looks like there's no way to set -fmath-errno at all). That's corroborated by looking at the LLVM IR for a call to sin which uses the non-errno setting LLVM intrinsic by default: https://godbolt.org/z/dvTvP3vPr.

clang/include/clang/Basic/DiagnosticDriverKinds.td

clang/lib/Driver/ToolChains/Clang.cpp

These two veclibs are only available for AArch64 targets, and as mentioned in https://discourse.llvm.org/t/rfc-should-fveclib-imply-fno-math-errno-for-all-targets/81384, we (Arm) think that `-fveclib` should imply `-fno-math-errno`. By setting `-fveclib` the user shows they intend to use the vector math functions, which implies they don't care about errno. However, currently, the vector mappings won't be used in many cases without setting `-fno-math-errno` separately. Making this change would also help resolve some inconsistencies in how vector mappings are applied (see llvm#108980 (comment)).

clang/include/clang/Driver/Options.td

kiranchandramohan

LG.

Changed in llvm#112580.

…#113673) Changed in #112580.

These two veclibs are only available for AArch64 targets, and as mentioned in https://discourse.llvm.org/t/rfc-should-fveclib-imply-fno-math-errno-for-all-targets/81384, we (Arm) think that `-fveclib` should imply `-fno-math-errno`. By setting `-fveclib` the user shows they intend to use the vector math functions, which implies they don't care about errno. However, currently, the vector mappings won't be used in many cases without setting `-fno-math-errno` separately. Making this change would also help resolve some inconsistencies in how vector mappings are applied (see llvm#108980 (comment)). Note: Both SLEEF and ArmPL state that they do not set `errno`: - https://developer.arm.com/documentation/101004/2410/General-information/Arm-Performance-Libraries-math-functions * "The vector functions in libamath which are available on Linux may not set errno nor raise exceptions" - https://sleef.org/2-references/libm/ * "These functions do not set errno nor raise an exception."

…llvm#113673) Changed in llvm#112580.

MacDue requested a review from mgabka October 16, 2024 16:44

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Oct 16, 2024

MacDue requested review from paulwalker-arm and kiranchandramohan October 16, 2024 16:44

MacDue requested a review from pawosm-arm October 16, 2024 16:45

kiranchandramohan reviewed Oct 16, 2024

View reviewed changes

kiranchandramohan requested a review from DavidTruby October 16, 2024 18:20

mgabka reviewed Oct 17, 2024

View reviewed changes

llvmbot added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Oct 17, 2024

MacDue force-pushed the fveclib_ArmPL_SLEEF branch from 1e01038 to cab134c Compare October 17, 2024 17:38

paulwalker-arm reviewed Oct 18, 2024

View reviewed changes

clang/include/clang/Basic/DiagnosticDriverKinds.td Outdated Show resolved Hide resolved

MacDue force-pushed the fveclib_ArmPL_SLEEF branch 3 times, most recently from e51af49 to a47592b Compare October 21, 2024 08:43

paulwalker-arm reviewed Oct 21, 2024

View reviewed changes

clang/lib/Driver/ToolChains/Clang.cpp Show resolved Hide resolved

clang/lib/Driver/ToolChains/Clang.cpp Outdated Show resolved Hide resolved

paulwalker-arm approved these changes Oct 21, 2024

View reviewed changes

MacDue force-pushed the fveclib_ArmPL_SLEEF branch from 4cb478c to 9c5e9df Compare October 22, 2024 13:41

MacDue added 7 commits October 23, 2024 09:25

Fixups and tests

c30fe19

Add clang warning

078f93e

Add diag group

7611598

Fix autocomplete.c test

bab89a6

Fixup warning

5ce5c33

Fixups

2d57eca

MacDue force-pushed the fveclib_ArmPL_SLEEF branch from 9c5e9df to 2d57eca Compare October 23, 2024 09:25

kiranchandramohan reviewed Oct 23, 2024

View reviewed changes

clang/include/clang/Driver/Options.td Outdated Show resolved Hide resolved

Use HelpTextForVariants

ad802c4

kiranchandramohan approved these changes Oct 23, 2024

View reviewed changes

MacDue merged commit 1f9953c into llvm:main Oct 23, 2024
8 checks passed

frobtech mentioned this pull request Oct 25, 2024

p/libc fexcept t #113664

Closed

MacDue added a commit to MacDue/llvm-project that referenced this pull request Oct 25, 2024

[clang][doc] Add release note for changes to -fveclib={ArmPL,SLEEF}

fa75765

Changed in llvm#112580.

MacDue mentioned this pull request Oct 25, 2024

[clang][doc] Add release note for changes to -fveclib={ArmPL,SLEEF} #113673

Merged

MacDue added a commit that referenced this pull request Oct 25, 2024

[clang][doc] Add release note for changes to -fveclib={ArmPL,SLEEF} (…

43dda4b

…#113673) Changed in #112580.

NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024

[clang][doc] Add release note for changes to -fveclib={ArmPL,SLEEF} (…

e039e73

…llvm#113673) Changed in llvm#112580.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] Make -fveclib={ArmPL,SLEEF} imply -fno-math-errno #112580

[clang] Make -fveclib={ArmPL,SLEEF} imply -fno-math-errno #112580

MacDue commented Oct 16, 2024

llvmbot commented Oct 16, 2024 •

edited

Loading

kiranchandramohan left a comment

kiranchandramohan Oct 16, 2024

mgabka Oct 16, 2024

paulwalker-arm Oct 17, 2024

DavidTruby commented Oct 17, 2024

mgabka Oct 17, 2024

paulwalker-arm Oct 17, 2024

MacDue Oct 17, 2024 •

edited

Loading

paulwalker-arm Oct 17, 2024

MacDue Oct 17, 2024

paulwalker-arm Oct 17, 2024

MacDue Oct 17, 2024

mgabka Oct 18, 2024

MacDue Oct 18, 2024

MacDue commented Oct 17, 2024

kiranchandramohan left a comment


		// RUN: %clang -### --target=aarch64-linux-gnu -fveclib=ArmPL -flto %s 2>&1 \| FileCheck --check-prefix=CHECK-LTO-ARMPL %s
		// CHECK-LTO-ARMPL-NOT: "-fmath-errno"

	if (IsMathLibCall) {
	// TODO: Ideally, we should not use clang-specific language here,
	// but it's hard to provide meaningful yet generic advice.
	// Also, should this be guarded by allowExtraAnalysis() and/or be part
	// of the returned info from isFunctionVectorizable()?
	reportVectorizationFailure(
	"Found a non-intrinsic callsite",
	"library call cannot be vectorized. "
	"Try compiling with -fno-math-errno, -ffast-math, "
	"or similar flags",
	"CantVectorizeLibcall", ORE, TheLoop, CI);

[clang] Make -fveclib={ArmPL,SLEEF} imply -fno-math-errno #112580

[clang] Make -fveclib={ArmPL,SLEEF} imply -fno-math-errno #112580

Conversation

MacDue commented Oct 16, 2024

llvmbot commented Oct 16, 2024 • edited Loading

kiranchandramohan 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

DavidTruby commented Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MacDue Oct 17, 2024 • edited Loading

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

MacDue commented Oct 17, 2024

kiranchandramohan left a comment

Choose a reason for hiding this comment

llvmbot commented Oct 16, 2024 •

edited

Loading

MacDue Oct 17, 2024 •

edited

Loading