Skip to content
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.

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

[LTO] Fix Veclib flags correctly pass to LTO flags #78749

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

paschalis-mpeis
Copy link
Member

Flags -fveclib=name were not passed to LTO flags.
This pass fixes that by converting the -fveclib flags to their relevant names for opt's -vector-lib=name flags.

For example:
-fveclib=SLEEF would become -vector-library=sleefgnuabi and passed through the -plugin-opt flag.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Jan 19, 2024
@llvmbot
Copy link
Member

llvmbot commented Jan 19, 2024

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Paschalis Mpeis (paschalis-mpeis)

Changes

Flags -fveclib=name were not passed to LTO flags.
This pass fixes that by converting the -fveclib flags to their relevant names for opt's -vector-lib=name flags.

For example:
-fveclib=SLEEF would become -vector-library=sleefgnuabi and passed through the -plugin-opt flag.


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

2 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+22)
  • (modified) clang/test/Driver/fveclib.c (+28)
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 385f66f3782bc1..470145849e4187 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -783,6 +783,28 @@ void tools::addLTOOptions(const ToolChain &ToolChain, const ArgList &Args,
                                          "-generate-arange-section"));
   }
 
+  // Pass vector library arguments to LTO.
+  Arg *ArgVecLib = Args.getLastArg(options::OPT_fveclib);
+  if (ArgVecLib && ArgVecLib->getNumValues() == 1) {
+    // Map the vector library names from clang front-end to opt front-end. The
+    // values are taken from the TargetLibraryInfo class command line options.
+    std::optional<StringRef> OptVal =
+        llvm::StringSwitch<std::optional<StringRef>>(ArgVecLib->getValue())
+            .Case("Accelerate", "Accelerate")
+            .Case("LIBMVEC", "LIBMVEC-X86")
+            .Case("MASSV", "MASSV")
+            .Case("SVML", "SVML")
+            .Case("SLEEF", "sleefgnuabi")
+            .Case("Darwin_libsystem_m", "Darwin_libsystem_m")
+            .Case("ArmPL", "ArmPL")
+            .Case("none", "none")
+            .Default(std::nullopt);
+
+    if (OptVal)
+      CmdArgs.push_back(Args.MakeArgString(
+          Twine(PluginOptPrefix) + "-vector-library=" + OptVal.value()));
+  }
+
   // Try to pass driver level flags relevant to LTO code generation down to
   // the plugin.
 
diff --git a/clang/test/Driver/fveclib.c b/clang/test/Driver/fveclib.c
index e2a7619e9b89f7..8ce7bcbb67ae0b 100644
--- a/clang/test/Driver/fveclib.c
+++ b/clang/test/Driver/fveclib.c
@@ -31,3 +31,31 @@
 
 // RUN: %clang -fveclib=Accelerate %s -nodefaultlibs -target arm64-apple-ios8.0.0 -### 2>&1 | FileCheck --check-prefix=CHECK-LINK-NODEFAULTLIBS %s
 // CHECK-LINK-NODEFAULTLIBS-NOT: "-framework" "Accelerate"
+
+
+/* Verify that the correct vector library is passed to LTO flags. */
+
+
+// RUN: %clang -### -fveclib=none -flto %s -v 2>&1  | FileCheck -check-prefix CHECK-LTO-NOLIB %s
+// CHECK-LTO-NOLIB: "-plugin-opt=-vector-library=none"
+
+// RUN: %clang -### -fveclib=Accelerate -flto %s -v 2>&1  | FileCheck -check-prefix CHECK-LTO-ACCELERATE %s
+// CHECK-LTO-ACCELERATE: "-plugin-opt=-vector-library=Accelerate"
+
+// RUN: %clang -### -fveclib=LIBMVEC -flto %s -v 2>&1  | FileCheck -check-prefix CHECK-LTO-LIBMVEC %s
+// CHECK-LTO-LIBMVEC: "-plugin-opt=-vector-library=LIBMVEC-X86"
+
+// RUN: %clang -### -fveclib=MASSV -flto %s -v 2>&1  | FileCheck -check-prefix CHECK-LTO-MASSV %s
+// CHECK-LTO-MASSV: "-plugin-opt=-vector-library=MASSV"
+
+// RUN: not %clang -### -fveclib=SVML -flto %s -v 2>&1  | FileCheck -check-prefix CHECK-LTO-SVML %s
+// CHECK-LTO-SVML: "-plugin-opt=-vector-library=SVML"
+
+// RUN: %clang -### -fveclib=SLEEF -flto %s -v 2>&1  | FileCheck -check-prefix CHECK-LTO-SLEEF %s
+// CHECK-LTO-SLEEF: "-plugin-opt=-vector-library=sleefgnuabi"
+
+// RUN: %clang -### -fveclib=Darwin_libsystem_m -flto %s -v 2>&1  | FileCheck -check-prefix CHECK-LTO-DARWIN %s
+// CHECK-LTO-DARWIN: "-plugin-opt=-vector-library=Darwin_libsystem_m"
+
+// RUN: %clang -### -fveclib=ArmPL -flto %s -v 2>&1  | FileCheck -check-prefix CHECK-LTO-ARMPL %s
+// CHECK-LTO-ARMPL: "-plugin-opt=-vector-library=ArmPL"

clang/test/Driver/fveclib.c Outdated Show resolved Hide resolved
// Map the vector library names from clang front-end to opt front-end. The
// values are taken from the TargetLibraryInfo class command line options.
std::optional<StringRef> OptVal =
llvm::StringSwitch<std::optional<StringRef>>(ArgVecLib->getValue())
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to refactor and reuse existing TargetLibraryInfo code, i.e. create a common static function that maps the values so that it can be called in multiple places?

Copy link
Member Author

Choose a reason for hiding this comment

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

Reusing code for mapping the strings is a great suggestion, however, TargetLibraryInfo statically creates some cl::opt (command line options) and I'm not sure ATM what would be a clean way to do this.

I would suggest such change (if is doable in a clean way) to be dealt in a separate patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

the TLI belongs to llvm world, while clang driver from what I can see is only linked with following llvm components:
set(LLVM_LINK_COMPONENTS
BinaryFormat
MC
Object
Option
ProfileData
Support
TargetParser
WindowsDriver
)
so I don't think it is possible to do it.

Unfortunately at the early stage when the tools::addLTOOptions function is called the CodeGenOptions are not even created, perhaps we could have a function defined which does the strings checking in include/llvm/Analysis/TargetLibraryInfo.h and include it in the CommonArgs.cpp? a least everything would be in one place? what do you think @david-arm ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think that would work, i.e. having a static function in TargetLibraryInfo.h that can be called in two places and doesn't have a dependency on the component/library. Having said that, I won't hold this patch up for this if it's too difficult!

clang/test/Driver/fveclib.c Outdated Show resolved Hide resolved
@paschalis-mpeis paschalis-mpeis force-pushed the users/paschalis-mpeis/fix-pass-veclib-lto-flags branch from e1f9741 to c342eea Compare January 22, 2024 12:34
clang/test/Driver/fveclib.c Outdated Show resolved Hide resolved
Flags `-fveclib=name` were not passed to LTO flags.
This pass fixes that by converting the `-fveclib` flags to their
relevant names for opt's `-vector-lib=name` flags.

For example:
`-fveclib=SLEEF` would become `-vector-library=sleefgnuabi` and passed
through the -plugin-opt` flag.
Tests that do not call addLTOOptions were removed.
clang/test/Driver/fveclib.c Outdated Show resolved Hide resolved
@paschalis-mpeis paschalis-mpeis force-pushed the users/paschalis-mpeis/fix-pass-veclib-lto-flags branch from f82dadf to edb1833 Compare January 23, 2024 09:40
Copy link
Contributor

@mgabka mgabka left a comment

Choose a reason for hiding this comment

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

It LGTM, but probably worth to give some extra time in case anybody else thinks differently, it would be really good to have it on LLVM18 as well

Copy link
Contributor

@david-arm david-arm left a comment

Choose a reason for hiding this comment

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

LGTM as well!

@paschalis-mpeis paschalis-mpeis merged commit 03cf0e9 into main Jan 25, 2024
4 checks passed
@paschalis-mpeis paschalis-mpeis deleted the users/paschalis-mpeis/fix-pass-veclib-lto-flags branch January 25, 2024 09:29
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Jan 26, 2024
Flags `-fveclib=name` were not passed to LTO flags.
This pass fixes that by converting the `-fveclib` flags to their
relevant names for opt's `-vector-lib=name` flags.

For example:
`-fveclib=SLEEF` would become `-vector-library=sleefgnuabi` and passed
through the `-plugin-opt` flag.

(cherry picked from commit 03cf0e9)
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Jan 27, 2024
Flags `-fveclib=name` were not passed to LTO flags.
This pass fixes that by converting the `-fveclib` flags to their
relevant names for opt's `-vector-lib=name` flags.

For example:
`-fveclib=SLEEF` would become `-vector-library=sleefgnuabi` and passed
through the `-plugin-opt` flag.

(cherry picked from commit 03cf0e9)
tstellar pushed a commit that referenced this pull request Jan 27, 2024
Flags `-fveclib=name` were not passed to LTO flags.
This pass fixes that by converting the `-fveclib` flags to their
relevant names for opt's `-vector-lib=name` flags.

For example:
`-fveclib=SLEEF` would become `-vector-library=sleefgnuabi` and passed
through the `-plugin-opt` flag.

(cherry picked from commit 03cf0e9)
paschalis-mpeis added a commit that referenced this pull request Feb 2, 2024
Pull request #78749 introduced a fix for passing vector library flags
from clang frontend to opt when LTO is used.

The fix also covered flang, as it's handled in CommonArgs.cpp, but no
testing was added for flang.
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
Flags `-fveclib=name` were not passed to LTO flags.
This pass fixes that by converting the `-fveclib` flags to their
relevant names for opt's `-vector-lib=name` flags.

For example:
`-fveclib=SLEEF` would become `-vector-library=sleefgnuabi` and passed
through the `-plugin-opt` flag.

(cherry picked from commit 03cf0e9)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
Flags `-fveclib=name` were not passed to LTO flags.
This pass fixes that by converting the `-fveclib` flags to their
relevant names for opt's `-vector-lib=name` flags.

For example:
`-fveclib=SLEEF` would become `-vector-library=sleefgnuabi` and passed
through the `-plugin-opt` flag.

(cherry picked from commit 03cf0e9)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
Flags `-fveclib=name` were not passed to LTO flags.
This pass fixes that by converting the `-fveclib` flags to their
relevant names for opt's `-vector-lib=name` flags.

For example:
`-fveclib=SLEEF` would become `-vector-library=sleefgnuabi` and passed
through the `-plugin-opt` flag.

(cherry picked from commit 03cf0e9)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
Flags `-fveclib=name` were not passed to LTO flags.
This pass fixes that by converting the `-fveclib` flags to their
relevant names for opt's `-vector-lib=name` flags.

For example:
`-fveclib=SLEEF` would become `-vector-library=sleefgnuabi` and passed
through the `-plugin-opt` flag.

(cherry picked from commit 03cf0e9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants