This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Allow for multiple gpu modules during translation.
ClosedPublic

Authored by herhut on Apr 15 2020, 7:03 AM.

Details

Summary

This change makes the ModuleTranslation threadsafe by locking on the
LLVMContext. Furthermore, we now clone the llvm module into a new
context when compiling to PTX similar to what the OrcJit does.

Diff Detail

Event Timeline

herhut created this revision.Apr 15 2020, 7:03 AM
csigg accepted this revision.Apr 15 2020, 8:03 AM
csigg added inline comments.
mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h
29

Is this needed?

This revision is now accepted and ready to land.Apr 15 2020, 8:03 AM
herhut updated this revision to Diff 257731.Apr 15 2020, 8:26 AM

Rebase and mild cleanup.

herhut marked an inline comment as done.Apr 15 2020, 8:27 AM
herhut added inline comments.
mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h
29

No, a leftover from using std::mutex in an earlier version. Thanks!

ftynse accepted this revision.Apr 15 2020, 9:38 AM

I don't suppose there is a way to make this method only visible to ModuleTranslation...

mehdi_amini added inline comments.Apr 15 2020, 10:15 PM
mlir/lib/Conversion/GPUToCUDA/ConvertKernelFuncToCubin.cpp
114

Why don't you use the high level API the same way this is done in the ExecutionEngine?

// TODO(zinenko): Reevaluate model of ownership of LLVMContext in LLVMDialect.
SmallVector<char, 1> buffer;
{
  llvm::raw_svector_ostream os(buffer);
  WriteBitcodeToFile(*llvmModule, os);
}
llvm::MemoryBufferRef bufferRef(StringRef(buffer.data(), buffer.size()),
                                "cloned module buffer");
auto expectedModule = parseBitcodeFile(bufferRef, *ctx);
if (!expectedModule)
  return expectedModule.takeError();
std::unique_ptr<Module> deserModule = std::move(*expectedModule);
auto dataLayout = deserModule->getDataLayout();

I'd also like a TODO also here for Alex to actually fix: the fact that we have a LLVMContext tied to the LLVM dialect is really something we need to fix.

114

(It may be even worth extracting this logic in a helper by the way)

herhut updated this revision to Diff 258012.Apr 16 2020, 4:25 AM

Extract helper and rebase.

ftynse accepted this revision.Apr 16 2020, 4:36 AM
ftynse added inline comments.
mlir/lib/Conversion/GPUToCUDA/ConvertKernelFuncToCubin.cpp
114

I'd also like a TODO also here for Alex to actually fix: the fact that we have a LLVMContext tied to the LLVM dialect is really something we need to fix.

Yeah, this is the next big thing on my todo list. Let's see how many discussion we can have in parallel :)

This revision was automatically updated to reflect the committed changes.

This is causing TSAN failures. Looks like ConvertKernelFuncToCubin isn't thread safe. More specifically the call to LLVMInitializeNVPTXTargetInfo.

mlir/lib/Conversion/GPUToCUDA/ConvertKernelFuncToCubin.cpp:62:5

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
500

Can we just lock once at beginning?