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

Reinstate support for merging modules with non-const globals. #40252

Merged
merged 1 commit into from
Mar 30, 2021

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Mar 29, 2021

#36260 introduced a change in jl_merge_modules, now requiring const-ness of global variable declarations:

julia/src/jitlayers.cpp

Lines 947 to 948 in 40267bb

assert(dG->isDeclaration() || (dG->getInitializer() == sG->getInitializer() &&
dG->isConstant() && sG->isConstant()));

Meanwhile, in GPU land we don't really have proper globals (suggestions on how to implement those are appreciated, but with NVPTX we can't use LLVM's JITs), so we use functions emitting weak GVs to emulate globals like such:

@eval @inline exception_flag() =
    Base.llvmcall(
        $("""@exception_flag = weak externally_initialized global i$(WORD_SIZE) 0
             define i64 @entry() #0 {
                 %ptr = load i$(WORD_SIZE), i$(WORD_SIZE)* @exception_flag, align 8
                 ret i$(WORD_SIZE) %ptr
             }
             attributes #0 = { alwaysinline }
          """, "entry"), Ptr{Cvoid}, Tuple{})

We can have multiple calls to these kind of functions, emitting duplicate globals, which LLVM's IRMover happily links together when we finish emitting a function:

julia/src/codegen.cpp

Lines 7380 to 7387 in 40267bb

for (auto &Mod : ctx.llvmcall_modules) {
SmallVector<std::string, 1> Exports;
for (const auto &F: Mod->functions())
if (!F.isDeclaration())
Exports.push_back(F.getName().str());
if (Linker::linkModules(*jl_Module, std::move(Mod))) {
jl_error("Failed to link LLVM bitcode");
}

However, when the calls originate from different methods, jl_compile_workqueue ends up calling jl_merge_modules which doesn't support linking these together, triggering the assertion at the top of this issue instead.

Solutions that come to mind:

  • relax the assertion (this PR): I assume this would be the simplest change, especially wrt. backporting for 1.6.1
  • switch jl_merge_modules over to LLVM's IRMover, although there's probably a reason we aren't already doing that (@vtjnash?)
  • emit globals differently: suggestions welcome.

I do realize that the assertion is specific to how Base Julia emits LLVM global variables, but it'd be nice to get existing code working again, and I'm happy to work on a better fix after that.

@maleadt maleadt added regression Regression in behavior compared to a previous version gpu Affects running Julia on a GPU backport 1.6 Change should be backported to release-1.6 labels Mar 29, 2021
@maleadt maleadt requested a review from vtjnash March 29, 2021 12:57
@vtjnash
Copy link
Member

vtjnash commented Mar 29, 2021

Linker::linkModules finishes by deleting any Constant object from the Context that isn't in the module, so it is highly unsafe to call (unfortunately so, as it would otherwise be rather useful). Disabling the check seems okay though.

@maleadt
Copy link
Member Author

maleadt commented Mar 30, 2021

I guess we could add a flag to add that behavior? It also needed this patch:

diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp
index b7a9beeeeb..d51c91e18f 100644
--- a/llvm/lib/Linker/IRMover.cpp
+++ b/llvm/lib/Linker/IRMover.cpp
@@ -1051,7 +1051,8 @@ Expected<Constant *> IRLinker::linkGlobalValueProto(GlobalValue *SGV,
 /// referenced are in Dest.
 void IRLinker::linkGlobalVariable(GlobalVariable &Dst, GlobalVariable &Src) {
   // Figure out what the initializer looks like in the dest module.
-  Mapper.scheduleMapGlobalInitializer(Dst, *Src.getInitializer());
+  if (Src.hasInitializer())
+    Mapper.scheduleMapGlobalInitializer(Dst, *Src.getInitializer());
 }

Anyway, I'll merge this for now so that I can resume development on the linker PR.
CI failures unrelated.

@maleadt maleadt merged commit 06e7ee6 into master Mar 30, 2021
@maleadt maleadt deleted the tb/merge_nonconst_globals branch March 30, 2021 05:49
@maleadt maleadt removed the backport 1.6 Change should be backported to release-1.6 label Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gpu Affects running Julia on a GPU regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants