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.

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

Jump to bottom

JITModule: rework/fix __udivdi3 handling #8389

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

JITModule: rework/fix __udivdi3 handling #8389

wants to merge 2 commits into from

Conversation

LebedevRI
Copy link
Contributor

@LebedevRI LebedevRI commented Aug 11, 2024 •
edited
Loading

The original workaround is very partial,
and was not really working in my experience,
even after making it non-GCC specific.

Instead:

  1. Ensure that the library that actually provides that symbol (as per the compiler used!) is actually linked into. This was not enough still.
  2. Replace HalideJITMemoryManager hack with a more direct approach of actually telling the JIT the address of the symbol.
  3. While there, move the symbol's forward definition to outside of namespaces. It's a global symbol, it makes sense to place it there.

This makes python binding tests pass on i386,
and i'm really happy about that.

https://ci.debian.net/packages/h/halide/testing/i386/50395069/#L2243

Refs. llvm/llvm-project#61289
Inspired by root-project/root#13286

@LebedevRI LebedevRI force-pushed the python-binding-32-bit branch 2 times, most recently from 6b38b4e to 3c9e720 Compare August 11, 2024 02:26
LebedevRI
LebedevRI commented Aug 11, 2024
View reviewed changes
src/CMakeLists.txt
@@ -550,6 +550,19 @@ if (WITH_SERIALIZATION_JIT_ROUNDTRIP_TESTING)
target_compile_definitions(Halide PRIVATE WITH_SERIALIZATION_JIT_ROUNDTRIP_TESTING)
endif ()

if (NOT DEFINED Halide_COMPILER_BUILTIN_LIBRARY)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh, rebases aren't fun.

Copy link
Member

Choose a reason for hiding this comment

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

This is going to blow up on Windows since MSVC isn't going to understand the --print-libgcc-file-name flag. Under which circumstances exactly do we need to include the __udivdi3 symbol?

Similarly, there's an issue on macOS with it finding arguably the wrong builtins.

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/15.0.0/lib/darwin/libclang_rt.builtins-x86_64.a

Can this all be guarded by if (CMAKE_SIZEOF_VOID_P EQUAL 4 AND ... something ...)? LINUX AND CMAKE_CXX_COMPILER_ID STREQUAL "GNU", maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Under which circumstances exactly do we need to include the __udivdi3 symbol?

Whenever the JIT might produce it but libHalide happens not to be linked
to the library that provides it. I'm hoping/wondering if we can just ask the clang
that we've found and are using to compile some runtimes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this kind-of works, and finds the right builtins on osx now:
https://buildbot.halide-lang.org/master/#/builders/171/builds/117

ninja: Entering directory `/Users/halidenightly/build_bot/worker/halide-testbranch-main-llvm20-arm-64-osx-cmake/halide-build'
ninja: error: '/Users/halidenightly/build_bot/worker/llvm-20-arm-64-osx/llvm-install/lib/clang/20/lib/darwin/libclang_rt.osx.a', needed by 'src/libHalide.19.0.0.dylib', missing and no known rule to make it

Similarly for windows:
https://buildbot.halide-lang.org/master/#/builders/157/builds/117

ninja: Entering directory `C:/build_bot/worker/halide-testbranch-main-llvm20-x86-64-windows-cmake/halide-build'
ninja: error: 'libgcc.a', needed by 'bin/Halide.dll', missing and no known rule to make it

I have absolutely no clue about win/osx, but that looks like the compiler-rt subproject is not enabled,
or, in osx case, the clang should be configured to use "system" libgcc from xcode?

@LebedevRI LebedevRI marked this pull request as draft August 11, 2024 21:42
LebedevRI added a commit to LebedevRI/Halide that referenced this pull request Aug 11, 2024
@LebedevRI
JITModule: rework/fix __udivdi3 handling
9e1b50b 
The original workaround is very partial,
and was not really working in my experience,
even after making it non-GCC specific.

Instead:
1. Ensure that the library that actually provides that symbol
   (as per the compiler used!) is actually linked into.
   This was not enough still.
2. Replace `HalideJITMemoryManager` hack with a more direct approach
   of actually telling the JIT the address of the symbol.
3. While there, move the symbol's forward definition to outside
   of namespaces. It's a global symbol, it makes sense to place it there.

This makes python binding tests pass on i386,
and i'm really happy about that.

Refs. llvm/llvm-project#61289
Inspired by root-project/root#13286

Forwarded: halide#8389

Gbp-Pq: Name 0010-JITModule-rework-fix-__udivdi3-handling.patch
@LebedevRI LebedevRI marked this pull request as ready for review August 11, 2024 22:40
alexreinking
alexreinking requested changes Aug 13, 2024
View reviewed changes
Copy link
Member

@alexreinking alexreinking left a comment

Choose a reason for hiding this comment

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

Some more thought needs to be put into which library to link and when.

src/CMakeLists.txt
@@ -550,6 +550,19 @@ if (WITH_SERIALIZATION_JIT_ROUNDTRIP_TESTING)
target_compile_definitions(Halide PRIVATE WITH_SERIALIZATION_JIT_ROUNDTRIP_TESTING)
endif ()

if (NOT DEFINED Halide_COMPILER_BUILTIN_LIBRARY)
Copy link
Member

Choose a reason for hiding this comment

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

This is going to blow up on Windows since MSVC isn't going to understand the --print-libgcc-file-name flag. Under which circumstances exactly do we need to include the __udivdi3 symbol?

Similarly, there's an issue on macOS with it finding arguably the wrong builtins.

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/15.0.0/lib/darwin/libclang_rt.builtins-x86_64.a

Can this all be guarded by if (CMAKE_SIZEOF_VOID_P EQUAL 4 AND ... something ...)? LINUX AND CMAKE_CXX_COMPILER_ID STREQUAL "GNU", maybe?

src/CMakeLists.txt Show resolved Hide resolved
LebedevRI added a commit to LebedevRI/Halide that referenced this pull request Aug 13, 2024
@LebedevRI
JITModule: rework/fix __udivdi3 handling
f634312 
The original workaround is very partial,
and was not really working in my experience,
even after making it non-GCC specific.

Instead:
1. Ensure that the library that actually provides that symbol
   (as per the compiler used!) is actually linked into.
   This was not enough still.
2. Replace `HalideJITMemoryManager` hack with a more direct approach
   of actually telling the JIT the address of the symbol.
3. While there, move the symbol's forward definition to outside
   of namespaces. It's a global symbol, it makes sense to place it there.

This makes python binding tests pass on i386,
and i'm really happy about that.

Refs. llvm/llvm-project#61289
Inspired by root-project/root#13286

Forwarded: halide#8389

Gbp-Pq: Name 0010-JITModule-rework-fix-__udivdi3-handling.patch
@LebedevRI
Copy link
Contributor Author

Ok, how about this?

@LebedevRI
JITModule: rework/fix __udivdi3 handling
66cdfbf 
The original workaround is very partial,
and was not really working in my experience,
even after making it non-GCC specific.

Instead:
1. Ensure that the library that actually provides that symbol
   (as per the compiler used!) is actually linked into.
   This was not enough still.
2. Replace `HalideJITMemoryManager` hack with a more direct approach
   of actually telling the JIT the address of the symbol.
3. While there, move the symbol's forward definition to outside
   of namespaces. It's a global symbol, it makes sense to place it there.

This makes python binding tests pass on i386,
and i'm really happy about that.

Refs. llvm/llvm-project#61289
Inspired by root-project/root#13286

Forwarded: halide#8389

Gbp-Pq: Name 0010-JITModule-rework-fix-__udivdi3-handling.patch
@LebedevRI
Copy link
Contributor Author

buildbot/halide-testbranch-main-llvm20-x86-32-linux-cmake suggests this should also be passing CFLAGS so cross-compilation of the halide itself to another arch works, i guess.

But as far as i can tell (from https://buildbot.halide-lang.org/master/#/builders/164/builds/123/steps/8/logs/stdio),
that build job is achieved via

 environment:
  CC=gcc-9 -m32

Shouldn't that be

 environment:
  CC=gcc-9
  CFLAGS=-m32
  CXXFLAGS=-m32

Otherwise i'm not really sure how that can be detected.

@alexreinking alexreinking self-assigned this Sep 10, 2024
@steven-johnson
Copy link
Contributor

Is this still an active PR?

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

@alexreinking alexreinking alexreinking requested changes

@halidebuildbots halidebuildbots Awaiting requested review from halidebuildbots

Requested changes must be addressed to merge this pull request.

Assignees

@alexreinking alexreinking

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LebedevRI @steven-johnson @alexreinking