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

Reuse incremental JIT compilation for --image-codegen #50649

Merged
merged 2 commits into from
Jul 26, 2023

Conversation

pchintalapudi
Copy link
Member

We don't need to merge all of the workqueue modules when performing compilation with --image-codegen set, we just need the global variable initializers to be defined before they're used in one of the modules. Therefore we can do this by compiling all of the global variable initializers upfront, so that later references will link them properly.

@pchintalapudi pchintalapudi added compiler:codegen Generation of LLVM IR and native code compiler:llvm For issues that relate to LLVM labels Jul 23, 2023
// need to emit a separate module for the globals before any functions are compiled,
// to ensure that the globals are defined when they are compiled.
if (params.imaging) {
jl_ExecutionEngine->addModule(jl_get_globals_module(params.tsctx, params.imaging, params.DL, params.TargetTriple, params.globals));
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Suggested change
jl_ExecutionEngine->addModule(jl_get_globals_module(params.tsctx, params.imaging, params.DL, params.TargetTriple, params.globals));
auto lock = params.tsctx.getLock();
for (auto &global : params.globals) {
auto GV = global.second;
GV->setName(GV->getName() + "." + unique_counter++);
addGlobalMapping(GV->getName(), global.first);
}

Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

LGTM, other than jl_get_globals_module which needs to mangle names to something unique in the current jl_ExecutionEngine.

@pchintalapudi
Copy link
Member Author

That's actually handled elsewhere in julia_pgv, which uses the globalUniqueGeneratedNames counter when in imaging mode. Should we move that uniquing logic here?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jul 26, 2023

Ah right, so you can remove that extra setName call then for now, but still simplify that loop substantially by using addGlobalMapping directly. Eventually we should remove globalUniqueGeneratedNames since it can be instead handled better by the JIT like this, as a strictly local property.

            for (auto &global : params.globals)
                addGlobalMapping(global.second->getName(), global.first);

The addGlobalMapping also isn't quite the same as imaging mode yet anyways, since we should actually append them into an array in the module itself later with internal linkage, so that they are all completely anonymous, then do a simple copy afterwards to fill in that array when doing the final linking step to populate those values (like we do in staticdata.c).

That should be some of the last steps needed to make codegen itself pure, and make native code caching feasible based on the IR or bitcode hash.

@pchintalapudi
Copy link
Member Author

I don't think addGlobalMapping is helpful here, we want the pointer to be the value stored in the global rather than the address of the global. We still need to allocate memory to load from in the IR.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jul 26, 2023

Ah, right. That probably gets us even closer to the version where we actually do the same representation as image_codegen though, since we would already make the equivalent array for it:

            void **globals = (void**)malloc(sizeof(void*) * params.globals.size());
            size_t i = 0;
            for (auto &global : params.globals) {
                globals[i] = global.first;
                addGlobalMapping(global.second->getName(), &globals[i]);
                i++;
            }

Then all the remains is converting all of the global.second objects into offsets into this array, like is done in aotcompile to build the fvars list.

@pchintalapudi
Copy link
Member Author

I think the better route to go about this is to convert imaging mode to use direct pointers instead of offsets, and make the linker patch in those references at load time.

@pchintalapudi pchintalapudi merged commit ff14eaf into master Jul 26, 2023
6 checks passed
@pchintalapudi pchintalapudi deleted the pc/jit-imaging branch July 26, 2023 13:34
@vtjnash
Copy link
Sponsor Member

vtjnash commented Jul 27, 2023

I don't see how that would work well with caching, since we don't have names for those things to give the linker. The names have to be globally meaningful for it to work, but everything really should be self-contained in the module. We also want it to be efficient, and giant string pools to compute offsets at runtime is a lot less compact than simply having the ordered array in the first place.

pchintalapudi added a commit that referenced this pull request Aug 10, 2023
We don't need to merge all of the workqueue modules when performing
compilation with `--image-codegen` set, we just need the global variable
initializers to be defined before they're used in one of the modules.
Therefore we can do this by compiling all of the global variable
initializers upfront, so that later references will link them properly.

(cherry picked from commit ff14eaf)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:codegen Generation of LLVM IR and native code compiler:llvm For issues that relate to LLVM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants