Skip to content

Bump Julia to LLVM 16 #51720

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
Merged
merged 30 commits into from
Jan 10, 2024
Merged

Bump Julia to LLVM 16 #51720

merged 30 commits into from
Jan 10, 2024

Conversation

@vchuravy
Copy link
Member

@vchuravy vchuravy commented Oct 15, 2023 •
edited by gbaraldi
Loading

Current Status:

  • Self test of Julia-GCChecker :: MissingRoots.c
  • ASAN: error: <unknown>:0: Cannot represent a difference across sections
  • x86_64-apple-darwin fails in ranges
  • llvmpasses

@giordano giordano added building Build system, or building Julia or its dependencies external dependencies Involves LLVM, OpenBLAS, or other linked libraries JLLs compiler:llvm For issues that relate to LLVM labels Oct 16, 2023
@vchuravy vchuravy mentioned this pull request Oct 16, 2023
Merged
@vchuravy
Copy link
Member Author 

/home/vchuravy/src/julia2/src/jitlayers.cpp: In lambda function:
/home/vchuravy/src/julia2/src/jitlayers.cpp:1675:19: error: 'class llvm::LLVMContext' has no member named 'hasSetOpaquePointersValue'; did you mean 'setOpaquePointers'?
 1675 |         if (!ctx->hasSetOpaquePointersValue())
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~
      |                   setOpaquePointers
make[1]: *** [Makefile:250: jitlayers.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:97: julia-src-release] Error 2

cc: @maleadt

@vtjnash
Copy link
Member

vtjnash commented Oct 26, 2023

LLVM deleted this functionality, claimed NFC for the change

commit 4756f9ae0aa6c2a0b4231d93d5166c055f303603
Author: Nikita Popov <[email protected]>
Date:   Wed Jan 18 11:04:12 2023 +0100

    [LLVMContext] Remove hasSetOpaquePointersValue() API (NFC)

@vtjnash
Copy link
Member

vtjnash commented Oct 26, 2023

You now need to check OpaquePointersCL.getNumOccurrences() to get that info (like we do during initialization--you could probably memoize that check into a global variable in our library as well to make it easy on yourself)

@vchuravy @gbaraldi
@pchintalapudi @vtjnash
Adopt to LLVM 16
946b6a1 
Co-authored-by: Gabriel Baraldi <[email protected]>
Co-authored-by: Prem Chintalapudi <[email protected]>
Co-authored-by: Jameson Nash <[email protected]>
Base automatically changed from vc/llvm16 to master October 28, 2023 21:25
@vchuravy vchuravy mentioned this pull request Nov 2, 2023
Open
@gbaraldi
Copy link
Member

gbaraldi commented Nov 2, 2023

Checking the command line isn't enough, because we want to respect wheter GPUCompiler added a value or not.

@gbaraldi
Copy link
Member

@nanosoldier runbenchmarks()

@gbaraldi
Copy link
Member

@nanosoldier runtests()

maleadt
maleadt reviewed Dec 21, 2023
View reviewed changes
@maleadt maleadt mentioned this pull request Dec 21, 2023
Merged
@gbaraldi
Copy link
Member

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@gbaraldi
Copy link
Member

gbaraldi commented Jan 5, 2024

After lots of annoyances and setbacks, I think this is ready for review. PkgEval looks good. the AllocCheck segfault is weird but I'm taking a look at it.

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Jan 5, 2024
pchintalapudi
pchintalapudi approved these changes Jan 6, 2024
View reviewed changes
src/aotcompile.cpp
// functions. We do so after optimization to avoid cloning these functions.

// Float16 conversion routines
#if defined(_CPU_X86_64_) && defined(_OS_DARWIN_) && JL_LLVM_VERSION >= 160000
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this dependent on the target triple? Just to preserve the fiction that codegen is cross-compile capable.

src/cgutils.cpp Outdated

static bool for_each_uniontype_small(
std::function<void(unsigned, jl_datatype_t*)> f,
const std::function<void(unsigned, jl_datatype_t*)> &f,
Copy link
Member

Choose a reason for hiding this comment

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

Could use an llvm::function_ref here.

src/jitlayers.cpp
void SetOpaquePointer(LLVMContext &ctx) {
if (jl_opaque_ptrs_set)
return;
#ifndef JL_LLVM_OPAQUE_POINTERS
Copy link
Member

Choose a reason for hiding this comment

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

Is anyone actually using this build flag, given we have a runtime option for setting it as well? I'm in favor of just deleting.

@gbaraldi gbaraldi merged commit 8e4221f into master Jan 10, 2024
@gbaraldi gbaraldi deleted the vc/llvm16_bump branch January 10, 2024 02:47
@vchuravy
Copy link
Member Author

Congratulations on getting this over the line!

@IanButterworth IanButterworth removed the merge me PR is reviewed. Merge when all tests are passing label Jan 13, 2024
@vchuravy vchuravy mentioned this pull request Jan 22, 2024
Closed
@inkydragon inkydragon mentioned this pull request Jan 26, 2024
Merged
@gbaraldi gbaraldi mentioned this pull request Jul 3, 2024
Open
@KristofferC KristofferC mentioned this pull request Jul 24, 2024
Closed
@inkydragon inkydragon mentioned this pull request Jan 2, 2025
Merged
@adienes adienes mentioned this pull request Jan 29, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maleadt maleadt maleadt left review comments

@pchintalapudi pchintalapudi pchintalapudi approved these changes

@vtjnash vtjnash Awaiting requested review from vtjnash

Assignees

No one assigned

Labels

building Build system, or building Julia or its dependencies compiler:llvm For issues that relate to LLVM external dependencies Involves LLVM, OpenBLAS, or other linked libraries JLLs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@vchuravy @vtjnash @gbaraldi @nanosoldier @maleadt @pchintalapudi @giordano @IanButterworth