[clang][driver] When -fveclib=ArmPL flag is in use, always link against libamath #116432

pawosm-arm · 2024-11-15T20:03:34Z

Using -fveclib=ArmPL without -lamath likely effects in the link-time errors.

MacDue · 2024-11-15T20:35:56Z

Typo in PR title flang -> flag

pawosm-arm · 2024-11-15T20:52:26Z

Typo in PR title flang -> flag

Corrected both the title and the commit message. Thanks for spotting this!

llvmbot · 2024-11-16T06:02:19Z

@llvm/pr-subscribers-flang-driver
@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Paul Osmialowski (pawosm-arm)

Changes

Using -fveclib=ArmPL without -lamath likely effects in the link-time errors.

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

3 Files Affected:

(modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+10)
(modified) clang/test/Driver/fveclib.c (+11)
(modified) flang/test/Driver/fveclib.f90 (+11)

diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index cbba4289eb9450..913797dec123fc 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -490,6 +490,16 @@ void tools::AddLinkerInputs(const ToolChain &TC, const InputInfoList &Inputs,
     else
       A.renderAsInput(Args, CmdArgs);
   }
+  if (const Arg *A = Args.getLastArg(options::OPT_fveclib)) {
+    if (A->getNumValues() == 1) {
+      StringRef V = A->getValue();
+      if (V == "ArmPL") {
+        CmdArgs.push_back(Args.MakeArgString("-lamath"));
+        CmdArgs.push_back(Args.MakeArgString("-lm"));
+        addArchSpecificRPath(TC, Args, CmdArgs);
+      }
+    }
+  }
 }
 
 void tools::addLinkerCompressDebugSectionsOption(
diff --git a/clang/test/Driver/fveclib.c b/clang/test/Driver/fveclib.c
index 8b233b0023398f..738c27fafb0524 100644
--- a/clang/test/Driver/fveclib.c
+++ b/clang/test/Driver/fveclib.c
@@ -102,3 +102,14 @@
 /* Verify no warning when math-errno is re-enabled for a different veclib (that does not imply -fno-math-errno). */
 // RUN: %clang -### --target=aarch64-linux-gnu -fveclib=ArmPL -fmath-errno -fveclib=LIBMVEC %s 2>&1 | FileCheck --check-prefix=CHECK-REPEAT-VECLIB %s
 // CHECK-REPEAT-VECLIB-NOT: math errno enabled
+
+/* Verify that vectorized routines library is being linked in. */
+// RUN: %clang -### --target=aarch64-linux-gnu -fveclib=ArmPL %s 2>&1 | FileCheck --check-prefix=CHECK-LINKING-ARMPL %s
+// CHECK-LINKING-ARMPL: "-lamath"
+// CHECK-LINKING-ARMPL-SAME: "-lm"
+
+/* Verify that the RPATH is being set when needed. */
+// RUN: %clang -### --target=aarch64-linux-gnu -frtlib-add-rpath -fveclib=ArmPL %s 2>&1 | FileCheck --check-prefix=CHECK-RPATH-ARMPL %s
+// CHECK-RPATH-ARMPL: "-lamath"
+// CHECK-RPATH-ARMPL-SAME: "-lm"
+// CHECK-RPATH-ARMPL-SAME: "-rpath"
diff --git a/flang/test/Driver/fveclib.f90 b/flang/test/Driver/fveclib.f90
index 14c59b0616f828..60a711a0816d3d 100644
--- a/flang/test/Driver/fveclib.f90
+++ b/flang/test/Driver/fveclib.f90
@@ -30,3 +30,14 @@
 
 ! TODO: if we add support for -nostdlib or -nodefaultlibs we need to test that
 ! these prevent "-framework Accelerate" being added on Darwin
+
+! RUN: %flang -### --target=aarch64-linux-gnu -fveclib=ArmPL %s 2>&1 | FileCheck --check-prefix=CHECK-LINKING-ARMPL %s
+! CHECK-LINKING-ARMPL: "-lamath"
+! CHECK-LINKING-ARMPL-SAME: "-lm"
+
+! RUN: %flang -### --target=aarch64-linux-gnu -frtlib-add-rpath -fveclib=ArmPL %s 2>&1 | FileCheck --check-prefix=CHECK-RPATH-ARMPL %s
+! CHECK-RPATH-ARMPL: "-lamath"
+! CHECK-RPATH-ARMPL-SAME: "-lm"
+! We need to see "-rpath" at least twice, one for veclib, one for the Fortran runtime
+! CHECK-RPATH-ARMPL-SAME: "-rpath"
+! CHECK-RPATH-ARMPL-SAME: "-rpath"

tblah · 2024-11-18T11:13:25Z

clang/lib/Driver/ToolChains/CommonArgs.cpp

+        CmdArgs.push_back(Args.MakeArgString("-lamath"));
+        CmdArgs.push_back(Args.MakeArgString("-lm"));


do these flags and library names work on Windows?

I don't think that -fveclib= works on Windows in the first place. I will check though.
There should probably be an if windows ... check around these too regardless, in case we do enable it on future in Windows.

I suppose the actual answer to your question is no; Windows doesn't have a separate libm, and the logic for linking to amath will probably be more complicated because you need a version of it matching your libc (the debug/release/static/dynamic grid that Windows makes you deal with)

Personally I think things like needing to know the dependencies required by a third party library show why we shouldn't be baking third party libraries into the driver in the first place. How the library is built is not under llvm's control nor are the libraries part of a recognised operating system. Are there any existing examples where other third party libraries are baked in?

We actually do support this on Windows, I was wrong above. So we should not be breaking this for Windows users.

I suppose the other option is to say that if someone passes -fveclib=ArmPL they will also have to pass the right flags to link to ArmPL and libamath themselves, rather than doing anything in the driver automatically? Assuming that's what @paulwalker-arm means, I'd probably agree and say that should be on the user to sort out and not on the driver.

Flang and ArmPL both support Windows. I don't see why Windows should get second-class support. I would prefer to see this PR extended to also include any flags required on Windows.

This code is common for flang and clang (see the test cases).

@DavidTruby as an Windows expert, what flags would you recommend for Windows here?

This function you're changing here (AddLinkerInputs) isn't actually called on Windows so no need to add an exclusion there. ArmPL is also supported on MacOS, but there it should use the same flags as it does on Linux. So I think what you had before is fine.

You'll need something similar for supporting both clang and flang on Windows, probably somewhere around where the other Windows on Arm options are:

llvm-project/clang/lib/Driver/ToolChains/MSVC.cpp

Line 86 in 9270328

Based on a quick glance at the ArmPL docs for the library name, the flag you'll need here is:
"--dependent-lib=amath"

Flang already has a hack for a different veclib implementation on darwin

The difference with the one instance of doing this is that Accelerate is a core part of that environment, much like libm is a core part of the linux environment. libamath is not core to any environment. I recommend seeking advice from the wider clang community before landing this work.

This function you're changing here (AddLinkerInputs) isn't actually called on Windows

So much about putting 'Common' in the filename (CommonArgs.cpp)

I think you're confusing Visual Studio with Windows. This function is being called for Mingw and Cygwin but not for MSVC.

MinGW here doesn't really mean MinGW (I realise this is quite confusing). It more specifically means "using glibc, ld.bfd and libstdc++". This isn't supported on AArch64 at all, and is mostly being phased out on X86 too I believe. Only the "new style" mingw-w64 is supported on AArch64 (using libucrt and link.exe).

There's no Cygwin on AArch64 either.
I suppose more specifically I should have said that this function will never be called on Windows on Arm 🙂

kiranchandramohan · 2024-11-18T12:11:08Z

We probably have to provide a link to some documentation for libamath. May be something like the following one. https://developer.arm.com/documentation/102574/2410/Optimized-math-routines---libamath

pawosm-arm · 2024-11-19T16:57:14Z

We probably have to provide a link to some documentation for libamath. May be something like the following one. https://developer.arm.com/documentation/102574/2410/Optimized-math-routines---libamath

I'm always anxious to provide a link that contains numeric ID (like the one, 102574/2410) as it suggest a dynamically generated reference that may be outlived by the lifespan of the change we're introducing in this commit...

pawosm-arm · 2024-11-19T17:48:30Z

I've extended it with Windows and Darwin.

tblah

This looks good to me but please don't merge without agreement on this approach from the other reviewers.

pawosm-arm · 2024-11-24T22:22:32Z

As I prepared a piece of code which makes the -l flags being treated properly when read from a config file (so it won't result in unresolved symbols e.g. when used with -static), I realized that going the config file way still creates a problem: with -fveclib=ArmPL -lamath put into a config file, it will always be linked with libamath, even if the user decides to use -fveclib=none to override this. And situation of this kind can only be solved properly by the patch discussed in this PR.

And there's another rationale behind it I can think of: using -fopenmp does not require the user to add -lomp to the command line, it just happens automatically. The sheer logic demands that -fveclib=ArmPL should result in the same treatment. There is no other provider for ArmPL's veclib functions these days than libamath.

DavidTruby · 2024-11-25T11:16:54Z

with -fveclib=ArmPL -lamath put into a config file, it will always be linked with libamath, even if the user decides to use -fveclib=none

I think there might be a related issue with the approach in this patch too though, in that -fveclib=ArmPL will silently force linking to amath for all math functions, rather than just the veclib functions, if we add -lamath as in this patch. I guess this could be resolved by changing the order of the -l options (i.e. have -lm first, so that symbols from it are selected first)?

MaskRay · 2024-12-03T03:23:55Z

clang/lib/Driver/ToolChains/CommonArgs.cpp

@@ -490,6 +490,17 @@ void tools::AddLinkerInputs(const ToolChain &TC, const InputInfoList &Inputs,
    else
      A.renderAsInput(Args, CmdArgs);
  }
+  if (const Arg *A = Args.getLastArg(options::OPT_fveclib)) {
+    if (A->getNumValues() == 1) {


fveclib is a Joined option that takes exactly one value. getNumValues() == 1 is an unneeded test

MaskRay · 2024-12-03T03:24:47Z

clang/lib/Driver/ToolChains/CommonArgs.cpp

+    if (A->getNumValues() == 1) {
+      const llvm::Triple &Triple = TC.getTriple();
+      StringRef V = A->getValue();
+      if ((V == "ArmPL") && (Triple.isOSLinux() || Triple.isOSDarwin())) {


We prefer no parens for A == "xxx" &&

MaskRay · 2024-12-03T03:26:37Z

clang/test/Driver/fveclib.c

@@ -102,3 +102,17 @@
 /* Verify no warning when math-errno is re-enabled for a different veclib (that does not imply -fno-math-errno). */
 // RUN: %clang -### --target=aarch64-linux-gnu -fveclib=ArmPL -fmath-errno -fveclib=LIBMVEC %s 2>&1 | FileCheck --check-prefix=CHECK-REPEAT-VECLIB %s
 // CHECK-REPEAT-VECLIB-NOT: math errno enabled
+
+/* Verify that vectorized routines library is being linked in. */


For comments that are not RUN / CHECK lines, if you want to make them stand out, we prefer /// instead of /* */

pawosm-arm · 2024-12-03T14:24:10Z

I've added these suggested changes. Also, I was advised by the libamath team that the complicated relationship between libamath and libm needs more elaborate handling, hence additional -lm before -lamath (see comment that I've also added). Also I was advised to use --as-needed, so I wrapped it around push/pop-state.

MaskRay · 2024-12-06T03:18:13Z

clang/lib/Driver/ToolChains/CommonArgs.cpp

+        CmdArgs.push_back(Args.MakeArgString("--push-state"));
+        CmdArgs.push_back(Args.MakeArgString("--as-needed"));
+      }
+      CmdArgs.push_back(Args.MakeArgString("-lm"));


why -m -lamath -lm ?

if there is circular dependency, the conventional way is --start-group -lamth -lm --end-group

No no, it isn't simply a dependency. It's a matter of prioritizing system-libm over libamath which contains faster (and potentially less accurate) implementations of some of the libm functions. I was advised, these should not be used in normal circumstances (it is just unfortunate that for historical reasons, this veclib provider isn't just veclib provider; but for -fveclib=ArmPL there is no other, and likely there will be no other). From the other side, some of the libamath-only functions depend on libm functions that are not in libamath. Is it certain that start/end-group algorithm would look into libraries in-order preferring library first to provide looked upon symbol (e.g. when given --start-group -lm -lamath --end-group)? Besides, Darwin's default ld doesn't seem to support --start-group.

@MaskRay is this response sufficient?

If "It's a matter of prioritizing system-libm over libamath which contains faster (and potentially less accurate) implementations of some of the libm functions.", the comment above doesn't seem to mention this.

However, I am confused. Why cannot these duplicate symbols be removed from libamath so that the compiler driver doesn't have to play with circular -lm and -lamath dependency?

If the reason is that we want to support both old libm and new libm where libm contains faster implementations also in libamath, I expect this reasoning to be covered.

If "It's a matter of prioritizing system-libm over libamath which contains faster (and potentially less accurate) implementations of some of the libm functions.", the comment above doesn't seem to mention this.

Now it mentions.

However, I am confused. Why cannot these duplicate symbols be removed from libamath so that the compiler driver doesn't have to play with circular -lm and -lamath dependency?

If the reason is that we want to support both old libm and new libm where libm contains faster implementations also in libamath, I expect this reasoning to be covered.

This is purely historical reason. Libamath was meant to be libm replacement, being veclib provider for fveclib=ArmPL is just one more role of this lib. The user still needs to be able to specify -lamath to be able to choose those less accurate faster functions, and I've made this covered by now added extra checks in the test cases.

pawosm-arm · 2024-12-09T14:54:46Z

with -fveclib=ArmPL -lamath put into a config file, it will always be linked with libamath, even if the user decides to use -fveclib=none

I think there might be a related issue with the approach in this patch too though, in that -fveclib=ArmPL will silently force linking to amath for all math functions, rather than just the veclib functions, if we add -lamath as in this patch. I guess this could be resolved by changing the order of the -l options (i.e. have -lm first, so that symbols from it are selected first)?

I missed your comment, seems it's the same issue we were discussing with libamath team. They recommended to use -lm -lamath -lm chain of flags.

…st libamath Using `-fveclib=ArmPL` without libamath likely effects in the link-time errors.

pawosm-arm requested review from DavidTruby and MacDue November 15, 2024 20:03

pawosm-arm changed the title ~~[clang][driver] When -fveclib=ArmPL flang is in use, always link against libamath~~ [clang][driver] When -fveclib=ArmPL flag is in use, always link against libamath Nov 15, 2024

pawosm-arm force-pushed the lamath branch from 991304b to 0906f31 Compare November 15, 2024 20:51

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

pawosm-arm force-pushed the lamath branch 2 times, most recently from dde726a to 865831b Compare November 16, 2024 13:07

tblah reviewed Nov 18, 2024

View reviewed changes

kiranchandramohan requested review from paulwalker-arm and mgabka November 18, 2024 12:09

pawosm-arm force-pushed the lamath branch from 865831b to 392192f Compare November 18, 2024 16:02

DavidTruby requested a review from MaskRay November 19, 2024 13:04

pawosm-arm force-pushed the lamath branch from 392192f to 5614595 Compare November 19, 2024 17:48

tblah approved these changes Nov 19, 2024

View reviewed changes

MaskRay reviewed Dec 3, 2024

View reviewed changes

pawosm-arm force-pushed the lamath branch from 5614595 to 12654b6 Compare December 3, 2024 14:21

pawosm-arm requested a review from MaskRay December 5, 2024 17:29

MaskRay reviewed Dec 6, 2024

View reviewed changes

[clang][driver] When -fveclib=ArmPL flag is in use, always link again…

d19c8ea

…st libamath Using `-fveclib=ArmPL` without libamath likely effects in the link-time errors.

pawosm-arm force-pushed the lamath branch from 12654b6 to d19c8ea Compare December 10, 2024 19:42

MaskRay approved these changes Dec 11, 2024

View reviewed changes

pawosm-arm merged commit 03019c6 into llvm:main Dec 11, 2024
8 checks passed

		CmdArgs.push_back(Args.MakeArgString("-lamath"));
		CmdArgs.push_back(Args.MakeArgString("-lm"));

[clang][driver] When -fveclib=ArmPL flag is in use, always link against libamath #116432

[clang][driver] When -fveclib=ArmPL flag is in use, always link against libamath #116432

Uh oh!

Conversation

pawosm-arm commented Nov 15, 2024

Uh oh!

MacDue commented Nov 15, 2024

Uh oh!

pawosm-arm commented Nov 15, 2024

Uh oh!

llvmbot commented Nov 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidTruby Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

pawosm-arm Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidTruby Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawosm-arm Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiranchandramohan commented Nov 18, 2024

Uh oh!

pawosm-arm commented Nov 19, 2024

Uh oh!

pawosm-arm commented Nov 19, 2024

Uh oh!

tblah left a comment

Choose a reason for hiding this comment

Uh oh!

pawosm-arm commented Nov 24, 2024

Uh oh!

DavidTruby commented Nov 25, 2024

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!

pawosm-arm commented Dec 3, 2024

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!

MaskRay Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawosm-arm Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

llvmbot commented Nov 16, 2024 •

edited

Loading

DavidTruby Nov 18, 2024 •

edited

Loading

pawosm-arm Nov 18, 2024 •

edited

Loading

DavidTruby Nov 19, 2024 •

edited

Loading

pawosm-arm Nov 19, 2024 •

edited

Loading

MaskRay Dec 10, 2024 •

edited

Loading

pawosm-arm Dec 10, 2024 •

edited

Loading

pawosm-arm commented Dec 9, 2024 •

edited

Loading