Skip to content

[DirectX] Disable all libcalls for DXIL in TargetLibraryInfo.cpp #138991

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

Merged
merged 2 commits into from
May 12, 2025

Conversation

Icohedron
Copy link
Contributor

@Icohedron Icohedron commented May 7, 2025

Fixes #138787

To the best of my knowledge, DXIL does not (and should not) support any of the libcalls in TargetLibraryInfo.def.
Math libcalls are not used in HLSL and also do not have lowerings to DXIL. (The current implementation of math functions are done via intrinsics.)
If there is a mistake with disabling all libcalls, then the libcalls we need can be re-enabled in a follow-up PR.

Locally, the codegen tests for Clang HLSL and the DirectX backend appear to pass without any issues.
The offload test suite's clang-d3d12 tests are also passing locally (Unsupported: 31 (62.00%), Passed: 19 (38.00%))
The change this PR introduces also eliminates the problematic memcpy intrinsics operating with a non-constant length argument and addrspace(3) ptrs showing up when compiling DML shaders. The number of DML shaders compiling successfully is greater with this PR than without.

@llvmbot
Copy link
Member

llvmbot commented May 7, 2025

@llvm/pr-subscribers-llvm-analysis

Author: Deric C. (Icohedron)

Changes

Fixes #138787

To the best of my knowledge, DXIL does not (and should not) support any of the libcalls in TargetLibraryInfo.def.
Math libcalls are not used in HLSL and also do not have lowerings to DXIL. (The current implementation of math functions are done via intrinsics.)
If there is a mistake with disabling all libcalls, then the libcalls we need can be re-enabled in a follow-up PR.

Locally, the codegen tests for Clang HLSL and the DirectX backend appear to pass without any issues.
The offload test suite's clang-d3d12 tests are also passing locally.
The change this PR introduces also eliminates the problematic memcpy intrinsics operating with a non-constant length argument and addrspace(3) ptrs showing up when compiling DML shaders. The number of DML shaders compiling successfully is greater with this PR.


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

1 Files Affected:

  • (modified) llvm/lib/Analysis/TargetLibraryInfo.cpp (+5)
diff --git a/llvm/lib/Analysis/TargetLibraryInfo.cpp b/llvm/lib/Analysis/TargetLibraryInfo.cpp
index 3945dd4a8b55d..1934fb4e1fa26 100644
--- a/llvm/lib/Analysis/TargetLibraryInfo.cpp
+++ b/llvm/lib/Analysis/TargetLibraryInfo.cpp
@@ -205,6 +205,11 @@ static void initializeLibCalls(TargetLibraryInfoImpl &TLI, const Triple &T,
     return;
   }
 
+  if (T.isDXIL()) {
+    TLI.disableAllFunctions();
+    return;
+  }
+
   // memset_pattern{4,8,16} is only available on iOS 3.0 and Mac OS X 10.5 and
   // later. All versions of watchOS support it.
   if (T.isMacOSX()) {

Co-authored-by: Justin Bogner <[email protected]>
@Icohedron Icohedron force-pushed the dxil-targetlibraryinfo branch from 007a1ae to 27d707d Compare May 8, 2025 00:21
@farzonl
Copy link
Member

farzonl commented May 8, 2025

To the best of my knowledge, DXIL does not (and should not) support any of the libcalls

This is correct we do not support libcalls

If there is a mistake with disabling all libcalls, then the libcalls we need can be re-enabled in a follow-up PR.

We should not enable any libcalls if something breaks thats probably a bug. If say we some how ended up in SimplifyLibCalls.cpp replaceUnaryCall that replaces libcalls with intrinsics that would still be wrong we shouldn't have ended up as a libcall to begin with.

The change this PR introduces also eliminates the problematic memcpy intrinsics

This seems to be more a side effect of how a few optimizations have been written than a defintitive stop all memcpys for example llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp says:

   // Don't introduce calls to memcpy/memmove intrinsics out of thin air if
  // the corresponding libcalls are not available.

// Don't introduce calls to memcpy/memmove intrinsics out of thin air if
// the corresponding libcalls are not available.
// TODO: We should really distinguish between libcall availability and
// our ability to introduce intrinsics.
if (T->isAggregateType() &&
(EnableMemCpyOptWithoutLibcalls ||
(TLI->has(LibFunc_memcpy) && TLI->has(LibFunc_memmove)))) {

All that said we should still do this change, because we don't want libcalls but I want to emphasize that I don't think this solves the memcpy problem in an intentional way.

@Icohedron Icohedron merged commit 6b7b289 into llvm:main May 12, 2025
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DirectX] Disable libcalls that we don't support
4 participants