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

release/18.x: Prepend all library intrinsics with # when building for Arm64EC #88016

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

dpaoliello
Copy link
Contributor

Backports #87542 to v18

While attempting to build some Rust code, I was getting linker errors due to missing functions that are implemented in compiler-rt. Turns out that when compiler-rt is built for Arm64EC, all its function names are mangled with the leading #.

This change removes the hard-coded list of library-implemented intrinsics to mangle for Arm64EC, and instead assumes that they all must be mangled.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 8, 2024

@llvm/pr-subscribers-backend-aarch64

Author: Daniel Paoliello (dpaoliello)

Changes

Backports #87542 to v18

While attempting to build some Rust code, I was getting linker errors due to missing functions that are implemented in compiler-rt. Turns out that when compiler-rt is built for Arm64EC, all its function names are mangled with the leading #.

This change removes the hard-coded list of library-implemented intrinsics to mangle for Arm64EC, and instead assumes that they all must be mangled.


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

2 Files Affected:

  • (modified) llvm/lib/Target/AArch64/AArch64ISelLowering.cpp (+8-34)
  • (modified) llvm/lib/Target/AArch64/AArch64ISelLowering.h (+3)
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
index 196aa50cf4060b..95d8ab95b2c097 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -1658,40 +1658,14 @@ AArch64TargetLowering::AArch64TargetLowering(const TargetMachine &TM,
   setMaxAtomicSizeInBitsSupported(128);
 
   if (Subtarget->isWindowsArm64EC()) {
-    // FIXME: are there other intrinsics we need to add here?
-    setLibcallName(RTLIB::MEMCPY, "#memcpy");
-    setLibcallName(RTLIB::MEMSET, "#memset");
-    setLibcallName(RTLIB::MEMMOVE, "#memmove");
-    setLibcallName(RTLIB::REM_F32, "#fmodf");
-    setLibcallName(RTLIB::REM_F64, "#fmod");
-    setLibcallName(RTLIB::FMA_F32, "#fmaf");
-    setLibcallName(RTLIB::FMA_F64, "#fma");
-    setLibcallName(RTLIB::SQRT_F32, "#sqrtf");
-    setLibcallName(RTLIB::SQRT_F64, "#sqrt");
-    setLibcallName(RTLIB::CBRT_F32, "#cbrtf");
-    setLibcallName(RTLIB::CBRT_F64, "#cbrt");
-    setLibcallName(RTLIB::LOG_F32, "#logf");
-    setLibcallName(RTLIB::LOG_F64, "#log");
-    setLibcallName(RTLIB::LOG2_F32, "#log2f");
-    setLibcallName(RTLIB::LOG2_F64, "#log2");
-    setLibcallName(RTLIB::LOG10_F32, "#log10f");
-    setLibcallName(RTLIB::LOG10_F64, "#log10");
-    setLibcallName(RTLIB::EXP_F32, "#expf");
-    setLibcallName(RTLIB::EXP_F64, "#exp");
-    setLibcallName(RTLIB::EXP2_F32, "#exp2f");
-    setLibcallName(RTLIB::EXP2_F64, "#exp2");
-    setLibcallName(RTLIB::EXP10_F32, "#exp10f");
-    setLibcallName(RTLIB::EXP10_F64, "#exp10");
-    setLibcallName(RTLIB::SIN_F32, "#sinf");
-    setLibcallName(RTLIB::SIN_F64, "#sin");
-    setLibcallName(RTLIB::COS_F32, "#cosf");
-    setLibcallName(RTLIB::COS_F64, "#cos");
-    setLibcallName(RTLIB::POW_F32, "#powf");
-    setLibcallName(RTLIB::POW_F64, "#pow");
-    setLibcallName(RTLIB::LDEXP_F32, "#ldexpf");
-    setLibcallName(RTLIB::LDEXP_F64, "#ldexp");
-    setLibcallName(RTLIB::FREXP_F32, "#frexpf");
-    setLibcallName(RTLIB::FREXP_F64, "#frexp");
+    // FIXME: are there intrinsics we need to exclude from this?
+    for (int i = 0; i < RTLIB::UNKNOWN_LIBCALL; ++i) {
+      auto code = static_cast<RTLIB::Libcall>(i);
+      auto libcallName = getLibcallName(code);
+      if ((libcallName != nullptr) && (libcallName[0] != '#')) {
+        setLibcallName(code, Saver.save(Twine("#") + libcallName).data());
+      }
+    }
   }
 }
 
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.h b/llvm/lib/Target/AArch64/AArch64ISelLowering.h
index 541a810fb5cba0..74d0c4bde8dd2e 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.h
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.h
@@ -1001,6 +1001,9 @@ class AArch64TargetLowering : public TargetLowering {
   /// make the right decision when generating code for different targets.
   const AArch64Subtarget *Subtarget;
 
+  llvm::BumpPtrAllocator BumpAlloc;
+  llvm::StringSaver Saver{BumpAlloc};
+
   bool isExtFreeImpl(const Instruction *Ext) const override;
 
   void addTypeForNEON(MVT VT);

@efriedma-quic
Copy link
Collaborator

LGTM. (This only affects Arm64EC, so it's very safe to backport.)

@dpaoliello dpaoliello added this to the LLVM 18.X Release milestone Apr 8, 2024
@dpaoliello dpaoliello changed the title Backport: Prepend all library intrinsics with # when building for Arm64EC release/18.x: Prepend all library intrinsics with # when building for Arm64EC Apr 11, 2024
@dpaoliello
Copy link
Contributor Author

@tstellar this is ready to land for 18

…vm#87542)

While attempting to build some Rust code, I was getting linker errors
due to missing functions that are implemented in `compiler-rt`. Turns
out that when `compiler-rt` is built for Arm64EC, all its function names
are mangled with the leading `#`.

This change removes the hard-coded list of library-implemented
intrinsics to mangle for Arm64EC, and instead assumes that they all must
be mangled.
@tstellar tstellar merged commit 4056cc2 into llvm:release/18.x Apr 15, 2024
9 of 10 checks passed
@dpaoliello dpaoliello deleted the arm64ecintrin18 branch April 15, 2024 23:04
@tstellar
Copy link
Collaborator

Hi @dpaoliello (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix.

@dpaoliello
Copy link
Contributor Author

Hi @dpaoliello (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix.

Fixes an issue with Arm64EC code generation where calls to some intrinsics implemented in compiler-rt used the wrong name mangling, eventually resulting in unresolved symbol errors during linking.

@tstellar
Copy link
Collaborator

tstellar commented May 7, 2024

Hi @dpaoliello I'm seeing some "Illegal Instruction" errors when running the bolt tests on aarch64. Do you think there is any chance this commit could be the cause? It's the only one between 18.1.3 and 18.1.4 that touches the aarch64 code gen. Here is the full log:

https://kojipkgs.fedoraproject.org//work/tasks/3490/117353490/build.log

@dpaoliello
Copy link
Contributor Author

Hi @dpaoliello I'm seeing some "Illegal Instruction" errors when running the bolt tests on aarch64. Do you think there is any chance this commit could be the cause? It's the only one between 18.1.3 and 18.1.4 that touches the aarch64 code gen. Here is the full log:

https://kojipkgs.fedoraproject.org//work/tasks/3490/117353490/build.log

@tstellar I wouldn't think so: the code path being changed here is guarded by a check for the Arm64EC subtarget on Windows, so it should never be triggered when building for AArch64 Linux. Additionally, if somehow this code was triggered, it should only result in different names for intrinsics (leading to unresolved symbols) rather than any change to which instruction is emitted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants