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

[clang][Driver] Pass -machine argument to the linker explicitly for ARM64EC targets. #86835

Merged
merged 1 commit into from
Mar 30, 2024

Conversation

cjacek
Copy link
Contributor

@cjacek cjacek commented Mar 27, 2024

Currently, Clang doesn't explicitly pass the -machine argument to the linker, relying on the linker to infer it from the input. However, MSVC's link.exe requires -machine to be specified explicitly for ARM64EC/ARM64X targets. Although lld-link allows inferring the target from ARM64EC object files, this detection isn't entirely reliable as AMD64 object files are also valid input for ARM64EC target.

Handling of -machine:arm64ec is straightforward. For ARM64X, this PR extends the ARM64EC target triple to accept the "arm64x" subarch string, which serves as an alias to "arm64ec" in most cases. However, during linker invocation, "arm64x" distinguishes between -machine:arm64ec and -machine:arm64x.

The only analogue to MSVC's behavior is cl.exe -arm64EC, which is handled by -machine:arm64ec part of this PR. As far as I can tell, linking ARM64X images with MSVC requires a direct link.exe invocation. In cases like lib.exe (#85972) or some
aspects of link.exe, -machine:arm64x serves as an alias to -machine:arm64ec too.

(BTW, meantime, I worked around it with -Wl,-machine:...).
CC @bylaws

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

llvmbot commented Mar 27, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Jacek Caban (cjacek)

Changes

Currently, Clang doesn't explicitly pass the -machine argument to the linker, relying on the linker to infer it from the input. However, MSVC's link.exe requires -machine to be specified explicitly for ARM64EC/ARM64X targets. Although lld-link allows inferring the target from ARM64EC object files, this detection isn't entirely reliable as AMD64 object files are also valid input for ARM64EC target.

Handling of -machine:arm64ec is straightforward. For ARM64X, this PR extends the ARM64EC target triple to accept the "arm64x" subarch string, which serves as an alias to "arm64ec" in most cases. However, during linker invocation, "arm64x" distinguishes between -machine:arm64ec and -machine:arm64x.

The only analogue to MSVC's behavior is cl.exe -arm64EC, which is handled by -machine:arm64ec part of this PR. As far as I can tell, linking ARM64X images with MSVC requires a direct link.exe invocation. In cases like lib.exe (#85972) or some
aspects of link.exe, -machine:arm64x serves as an alias to -machine:arm64ec too.

(BTW, meantime, I worked around it with -Wl,-machine:...).
CC @bylaws


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

6 Files Affected:

  • (modified) clang/lib/Driver/Driver.cpp (+1-2)
  • (modified) clang/lib/Driver/ToolChains/MSVC.cpp (+7)
  • (modified) clang/test/Driver/msvc-link.c (+6)
  • (modified) llvm/include/llvm/TargetParser/Triple.h (+3-1)
  • (modified) llvm/lib/TargetParser/Triple.cpp (+5-4)
  • (modified) llvm/unittests/TargetParser/TripleTest.cpp (+25)
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 7a53764364ce4d..328ea93b679293 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1458,8 +1458,7 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
   }
 
   // Report warning when arm64EC option is overridden by specified target
-  if ((TC.getTriple().getArch() != llvm::Triple::aarch64 ||
-       TC.getTriple().getSubArch() != llvm::Triple::AArch64SubArch_arm64ec) &&
+  if (!TC.getTriple().isWindowsArm64EC() &&
       UArgs->hasArg(options::OPT__SLASH_arm64EC)) {
     getDiags().Report(clang::diag::warn_target_override_arm64ec)
         << TC.getTriple().str();
diff --git a/clang/lib/Driver/ToolChains/MSVC.cpp b/clang/lib/Driver/ToolChains/MSVC.cpp
index dc534a33e6d0ef..edf81ce7e425be 100644
--- a/clang/lib/Driver/ToolChains/MSVC.cpp
+++ b/clang/lib/Driver/ToolChains/MSVC.cpp
@@ -79,6 +79,13 @@ void visualstudio::Linker::ConstructJob(Compilation &C, const JobAction &JA,
     CmdArgs.push_back(
         Args.MakeArgString(std::string("-out:") + Output.getFilename()));
 
+  if (TC.getTriple().isWindowsArm64EC()) {
+    if (TC.getTriple().getSubArch() == llvm::Triple::AArch64SubArch_arm64x)
+      CmdArgs.push_back("-machine:arm64x");
+    else
+      CmdArgs.push_back("-machine:arm64ec");
+  }
+
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles) &&
       !C.getDriver().IsCLMode() && !C.getDriver().IsFlangMode()) {
     CmdArgs.push_back("-defaultlib:libcmt");
diff --git a/clang/test/Driver/msvc-link.c b/clang/test/Driver/msvc-link.c
index 64e099ea63042f..03ba3cd5805188 100644
--- a/clang/test/Driver/msvc-link.c
+++ b/clang/test/Driver/msvc-link.c
@@ -36,3 +36,9 @@
 // VFSOVERLAY: "--vfsoverlay"
 // VFSOVERLAY: lld-link
 // VFSOVERLAY: "/vfsoverlay:{{.*}}" "{{.*}}.obj"
+
+// RUN: %clang -target arm64ec-pc-windows-msvc -fuse-ld=link -### %s 2>&1 | FileCheck --check-prefix=ARM64EC %s
+// ARM64EC: "-machine:arm64ec"
+
+// RUN: %clang -target arm64x-pc-windows-msvc -fuse-ld=link -### %s 2>&1 | FileCheck --check-prefix=ARM64X %s
+// ARM64X: "-machine:arm64x"
diff --git a/llvm/include/llvm/TargetParser/Triple.h b/llvm/include/llvm/TargetParser/Triple.h
index f256e2b205a889..fb55011d68c6bf 100644
--- a/llvm/include/llvm/TargetParser/Triple.h
+++ b/llvm/include/llvm/TargetParser/Triple.h
@@ -148,6 +148,7 @@ class Triple {
 
     AArch64SubArch_arm64e,
     AArch64SubArch_arm64ec,
+    AArch64SubArch_arm64x,
 
     KalimbaSubArch_v3,
     KalimbaSubArch_v4,
@@ -623,7 +624,8 @@ class Triple {
   // Checks if we're using the Windows Arm64EC ABI.
   bool isWindowsArm64EC() const {
     return getArch() == Triple::aarch64 &&
-           getSubArch() == Triple::AArch64SubArch_arm64ec;
+           (getSubArch() == Triple::AArch64SubArch_arm64ec ||
+            getSubArch() == Triple::AArch64SubArch_arm64x);
   }
 
   bool isWindowsCoreCLREnvironment() const {
diff --git a/llvm/lib/TargetParser/Triple.cpp b/llvm/lib/TargetParser/Triple.cpp
index 624679ff507a7f..29c634eec7866a 100644
--- a/llvm/lib/TargetParser/Triple.cpp
+++ b/llvm/lib/TargetParser/Triple.cpp
@@ -110,7 +110,7 @@ StringRef Triple::getArchName(ArchType Kind, SubArchType SubArch) {
       return "mipsisa64r6el";
     break;
   case Triple::aarch64:
-    if (SubArch == AArch64SubArch_arm64ec)
+    if (SubArch == AArch64SubArch_arm64ec || SubArch == AArch64SubArch_arm64x)
       return "arm64ec";
     if (SubArch == AArch64SubArch_arm64e)
       return "arm64e";
@@ -515,10 +515,8 @@ static Triple::ArchType parseArch(StringRef ArchName) {
     .Case("aarch64_be", Triple::aarch64_be)
     .Case("aarch64_32", Triple::aarch64_32)
     .Case("arc", Triple::arc)
-    .Case("arm64", Triple::aarch64)
+    .Cases("arm64", "arm64e", "arm64ec", "arm64x", Triple::aarch64)
     .Case("arm64_32", Triple::aarch64_32)
-    .Case("arm64e", Triple::aarch64)
-    .Case("arm64ec", Triple::aarch64)
     .Case("arm", Triple::arm)
     .Case("armeb", Triple::armeb)
     .Case("thumb", Triple::thumb)
@@ -729,6 +727,9 @@ static Triple::SubArchType parseSubArch(StringRef SubArchName) {
   if (SubArchName == "arm64ec")
     return Triple::AArch64SubArch_arm64ec;
 
+  if (SubArchName == "arm64x")
+    return Triple::AArch64SubArch_arm64x;
+
   if (SubArchName.starts_with("spirv"))
     return StringSwitch<Triple::SubArchType>(SubArchName)
         .EndsWith("v1.0", Triple::SPIRVSubArch_v10)
diff --git a/llvm/unittests/TargetParser/TripleTest.cpp b/llvm/unittests/TargetParser/TripleTest.cpp
index e1e1bbd76ecda6..76a22c5995c42b 100644
--- a/llvm/unittests/TargetParser/TripleTest.cpp
+++ b/llvm/unittests/TargetParser/TripleTest.cpp
@@ -2226,6 +2226,16 @@ TEST(TripleTest, ParseARMArch) {
     T.setArch(Triple::aarch64, Triple::AArch64SubArch_arm64ec);
     EXPECT_EQ("arm64ec", T.getArchName());
   }
+  {
+    Triple T = Triple("arm64x");
+    EXPECT_EQ(Triple::aarch64, T.getArch());
+    EXPECT_EQ(Triple::AArch64SubArch_arm64x, T.getSubArch());
+  }
+  {
+    Triple T;
+    T.setArch(Triple::aarch64, Triple::AArch64SubArch_arm64x);
+    EXPECT_EQ("arm64ec", T.getArchName());
+  }
 }
 
 TEST(TripleTest, isArmT32) {
@@ -2371,4 +2381,19 @@ TEST(TripleTest, isArmMClass) {
     EXPECT_TRUE(T.isArmMClass());
   }
 }
+
+TEST(TripleTest, isWindowsArm64EC) {
+  {
+    Triple T = Triple("arm64ec");
+    EXPECT_TRUE(T.isWindowsArm64EC());
+  }
+  {
+    Triple T = Triple("arm64x");
+    EXPECT_TRUE(T.isWindowsArm64EC());
+  }
+  {
+    Triple T = Triple("aarch64");
+    EXPECT_FALSE(T.isWindowsArm64EC());
+  }
+}
 } // end anonymous namespace

Copy link

github-actions bot commented Mar 27, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@efriedma-quic
Copy link
Collaborator

arm64x isn't really a "target" in the sense that you can build code for it. When you're building a C file, it's either arm64, or arm64ec; it can't be "arm64x". arm64x is just a library format that combines an arm64 library with an arm64ec library.

So I'm tempted to say we should just add a separate flag for this, instead of overloading the triple. "-marm64x" or something like that.

@cjacek
Copy link
Contributor Author

cjacek commented Mar 28, 2024

Sounds good to me, the new version uses -marm64x flag instead. Thanks.

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

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

LGTM

@cjacek cjacek merged commit 37c175a into llvm:main Mar 30, 2024
4 checks passed
@cjacek cjacek deleted the clang-machine branch March 30, 2024 15:43
cjacek added a commit to cjacek/llvm-project that referenced this pull request Mar 30, 2024
Add '--' argument to clang-cl to avoid interpreting input files with /U option.
Fix for llvm-clang-aarch64-darwin buildbot failure after llvm#86835.
cjacek added a commit that referenced this pull request Mar 30, 2024
Add '--' argument to clang-cl to avoid interpreting input files with /U
option. Fix for llvm-clang-aarch64-darwin buildbot failure after #86835.
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.

None yet

3 participants