Skip to content

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

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

Merged
merged 1 commit into from
Dec 11, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 33 additions & 0 deletions clang/lib/Driver/ToolChains/CommonArgs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,39 @@ void tools::AddLinkerInputs(const ToolChain &TC, const InputInfoList &Inputs,
else
A.renderAsInput(Args, CmdArgs);
}
if (const Arg *A = Args.getLastArg(options::OPT_fveclib)) {
const llvm::Triple &Triple = TC.getTriple();
StringRef V = A->getValue();
if (V == "ArmPL" && (Triple.isOSLinux() || Triple.isOSDarwin())) {
// To support -fveclib=ArmPL we need to link against libamath. Some of the
// libamath functions depend on libm, at the same time, libamath exports
// its own implementation of some of the libm functions. These are faster
// and potentially less accurate implementations, hence we need to be
// careful what is being linked in. Since here we are interested only in
// the subset of libamath functions that is covered by the veclib
// mappings, we need to prioritize libm functions by putting -lm before
// -lamath (and then -lm again, to fulfill libamath requirements).
//
// Therefore we need to do the following:
//
// 1. On Linux, link only when actually needed.
//
// 2. Prefer libm functions over libamath.
//
// 3. Link against libm to resolve libamath dependencies.
//
if (Triple.isOSLinux()) {
CmdArgs.push_back(Args.MakeArgString("--push-state"));
CmdArgs.push_back(Args.MakeArgString("--as-needed"));
}
CmdArgs.push_back(Args.MakeArgString("-lm"));
Copy link
Member

Choose a reason for hiding this comment

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

why -m -lamath -lm ?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MaskRay is this response sufficient?

Copy link
Member

@MaskRay MaskRay Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@pawosm-arm pawosm-arm Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

CmdArgs.push_back(Args.MakeArgString("-lamath"));
CmdArgs.push_back(Args.MakeArgString("-lm"));
if (Triple.isOSLinux())
CmdArgs.push_back(Args.MakeArgString("--pop-state"));
addArchSpecificRPath(TC, Args, CmdArgs);
}
}
}

void tools::addLinkerCompressDebugSectionsOption(
Expand Down
6 changes: 6 additions & 0 deletions clang/lib/Driver/ToolChains/MSVC.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,12 @@ void visualstudio::Linker::ConstructJob(Compilation &C, const JobAction &JA,
else if (TC.getTriple().isWindowsArm64EC())
CmdArgs.push_back("-machine:arm64ec");

if (const Arg *A = Args.getLastArg(options::OPT_fveclib)) {
StringRef V = A->getValue();
if (V == "ArmPL")
CmdArgs.push_back(Args.MakeArgString("--dependent-lib=amath"));
}

if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles) &&
!C.getDriver().IsCLMode() && !C.getDriver().IsFlangMode()) {
CmdArgs.push_back("-defaultlib:libcmt");
Expand Down
17 changes: 17 additions & 0 deletions clang/test/Driver/fveclib.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,3 +112,20 @@
/* 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-pc-windows-msvc -fveclib=ArmPL %s 2>&1 | FileCheck --check-prefix=CHECK-LINKING-ARMPL-MSVC %s
// RUN: %clang -### --target=aarch64-linux-gnu -fveclib=ArmPL %s 2>&1 | FileCheck --check-prefix=CHECK-LINKING-ARMPL-LINUX %s
// RUN: %clang -### --target=aarch64-linux-gnu -fveclib=ArmPL %s -lamath 2>&1 | FileCheck --check-prefix=CHECK-LINKING-AMATH-BEFORE-ARMPL-LINUX %s
// RUN: %clang -### --target=arm64-apple-darwin -fveclib=ArmPL %s 2>&1 | FileCheck --check-prefix=CHECK-LINKING-ARMPL-DARWIN %s
// RUN: %clang -### --target=arm64-apple-darwin -fveclib=ArmPL %s -lamath 2>&1 | FileCheck --check-prefix=CHECK-LINKING-AMATH-BEFORE-ARMPL-DARWIN %s
// CHECK-LINKING-ARMPL-LINUX: "--push-state" "--as-needed" "-lm" "-lamath" "-lm" "--pop-state"
// CHECK-LINKING-ARMPL-DARWIN: "-lm" "-lamath" "-lm"
// CHECK-LINKING-ARMPL-MSVC: "--dependent-lib=amath"
// CHECK-LINKING-AMATH-BEFORE-ARMPL-LINUX: "-lamath" {{.*}}"--push-state" "--as-needed" "-lm" "-lamath" "-lm" "--pop-state"
// CHECK-LINKING-AMATH-BEFORE-ARMPL-DARWIN: "-lamath" {{.*}}"-lm" "-lamath" "-lm"

/// Verify that the RPATH is being set when needed.
// RUN: %clang -### --target=aarch64-linux-gnu -resource-dir=%S/../../../clang/test/Driver/Inputs/resource_dir_with_arch_subdir -frtlib-add-rpath -fveclib=ArmPL %s 2>&1 | FileCheck --check-prefix=CHECK-RPATH-ARMPL %s
// CHECK-RPATH-ARMPL: "--push-state" "--as-needed" "-lm" "-lamath" "-lm" "--pop-state"
// CHECK-RPATH-ARMPL-SAME: "-rpath"
17 changes: 17 additions & 0 deletions flang/test/Driver/fveclib.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,20 @@

! 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-pc-windows-msvc -fveclib=ArmPL %s 2>&1 | FileCheck --check-prefix=CHECK-LINKING-ARMPL-MSVC %s
! RUN: %flang -### --target=aarch64-linux-gnu -fveclib=ArmPL %s 2>&1 | FileCheck --check-prefix=CHECK-LINKING-ARMPL-LINUX %s
! RUN: %flang -### --target=aarch64-linux-gnu -fveclib=ArmPL %s -lamath 2>&1 | FileCheck --check-prefix=CHECK-LINKING-AMATH-BEFORE-ARMPL-LINUX %s
! RUN: %flang -### --target=arm64-apple-darwin -fveclib=ArmPL %s 2>&1 | FileCheck --check-prefix=CHECK-LINKING-ARMPL-DARWIN %s
! RUN: %flang -### --target=arm64-apple-darwin -fveclib=ArmPL %s -lamath 2>&1 | FileCheck --check-prefix=CHECK-LINKING-AMATH-BEFORE-ARMPL-DARWIN %s
! CHECK-LINKING-ARMPL-LINUX: "--push-state" "--as-needed" "-lm" "-lamath" "-lm" "--pop-state"
! CHECK-LINKING-ARMPL-DARWIN: "-lm" "-lamath" "-lm"
! CHECK-LINKING-ARMPL-MSVC: "--dependent-lib=amath"
! CHECK-LINKING-AMATH-BEFORE-ARMPL-LINUX: "-lamath" {{.*}}"--push-state" "--as-needed" "-lm" "-lamath" "-lm" "--pop-state"
! CHECK-LINKING-AMATH-BEFORE-ARMPL-DARWIN: "-lamath" {{.*}}"-lm" "-lamath" "-lm"

! RUN: %flang -### --target=aarch64-linux-gnu -resource-dir=%S/../../../clang/test/Driver/Inputs/resource_dir_with_arch_subdir -frtlib-add-rpath -fveclib=ArmPL %s 2>&1 | FileCheck --check-prefix=CHECK-RPATH-ARMPL %s
! CHECK-RPATH-ARMPL: "--push-state" "--as-needed" "-lm" "-lamath" "-lm" "--pop-state"
! 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"
Loading