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

Bump Julia to LLVM 16 #51720

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
Sponsor Member

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

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 domain:building Build system, or building Julia or its dependencies external dependencies Involves LLVM, OpenBLAS, or other linked libraries stdlib: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
Sponsor 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
Sponsor Member

vtjnash commented Oct 26, 2023

LLVM deleted this functionality, claimed NFC for the change

commit 4756f9ae0aa6c2a0b4231d93d5166c055f303603
Author: Nikita Popov <npopov@redhat.com>
Date:   Wed Jan 18 11:04:12 2023 +0100

    [LLVMContext] Remove hasSetOpaquePointersValue() API (NFC)

@vtjnash
Copy link
Sponsor 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 <baraldigabriel@gmail.com>
Co-authored-by: Prem Chintalapudi <prem.chintalapudi@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
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()

@gbaraldi gbaraldi force-pushed the vc/llvm16_bump branch 2 times, most recently from f9a68cc to f76032e Compare December 20, 2023 18:14
@gbaraldi @maleadt
Why do I even bother also thanks tim
471dc6c 
Co-authored-by: Tim Besard <tim.besard@gmail.com>
maleadt
maleadt reviewed Dec 21, 2023
View reviewed changes
src/jitlayers.cpp Outdated Show resolved Hide resolved
@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 status: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
@@ -1051,6 +1051,20 @@ static AOTOutputs add_output_impl(Module &M, TargetMachine &SourceTM, ShardTimer
// 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
@@ -911,7 +911,7 @@ static bool is_tupletype_homogeneous(jl_svec_t *t, bool allow_va = false)
}

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
4 of 7 checks passed
@gbaraldi gbaraldi deleted the vc/llvm16_bump branch January 10, 2024 02:47
@vchuravy
Copy link
Sponsor Member Author

Congratulations on getting this over the line!

@IanButterworth IanButterworth removed the status: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
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
compiler:llvm For issues that relate to LLVM domain:building Build system, or building Julia or its dependencies external dependencies Involves LLVM, OpenBLAS, or other linked libraries stdlib:JLLs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@vchuravy @vtjnash @gbaraldi @nanosoldier @maleadt @pchintalapudi @giordano @IanButterworth