This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Target][LLVM] Adds an utility class for serializing operations to binary strings.
ClosedPublic

Authored by fmorac on Jun 29 2023, 8:39 AM.

Details

Summary

For an explanation of these patches see D154153.

Commit message:
This patch adds the utility base class ModuleToObject. This class provides an
interface for compiling module operations into binary strings, by default this
class serialize modules to LLVM bitcode.

Diff Detail

Event Timeline

fmorac created this revision.Jun 29 2023, 8:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2023, 8:39 AM
fmorac updated this revision to Diff 536062.Jun 29 2023, 5:24 PM

Rebasing.

fmorac edited the summary of this revision. (Show Details)Jun 29 2023, 6:15 PM
fmorac added a reviewer: mehdi_amini.
fmorac published this revision for review.Jun 30 2023, 6:27 AM

Does this really belong to the ExecutionEngine? Seems like a more independent utility, but I'm not entirely sure where to slot this either.

mlir/include/mlir/ExecutionEngine/ModuleToObject.h
37 ↗(On Diff #536062)

Does this only work with gpu.module as the comment says?

Also this is dead code right now: could this be unit-tested somehow?

Does this really belong to the ExecutionEngine? Seems like a more independent utility, but I'm not entirely sure where to slot this either.

I was not sure were to put it either. I add it here because I know the execution engine can dump object code, so maybe some of these utilities could be used by the Execution Engine in the future?

Also this is dead code right now: could this be unit-tested somehow?

The best test I could think of is serializing a module and reading it back and checking the contents, however I don't think that would be a particularly good test.

As a general thought re organization, it might make sense to move this and also stuff like OptUtils from the execution engine to ... let's call it something like LLVMUsageUtils or LLVMCompilerUtils ... and that'd either be a standalone thing or something we'd stick under Support/ (or Target/LLVM/) by analogy to how LLVM keeps a lot of the OS-interaction wrappers in its Support library..

But maybe that's a very silly thought.

Targets/LLVM may be a good fit (as a new CMake library target in this directory).

fmorac updated this revision to Diff 538780.Jul 10 2023, 12:49 PM
fmorac marked an inline comment as done.

Moves the ModuleToObject class to Target/LLVM. Also adds an unit test.

I'm confused about one of the dependencies, but this is a good refactor otherwise

mlir/lib/Target/LLVM/CMakeLists.txt
14

This shouldn't need the native codegen?

20

Nit: newline

mlir/unittests/Target/LLVM/CMakeLists.txt
5

The dependency on native codegen might need to be here?

fmorac updated this revision to Diff 543106.EditedJul 21 2023, 3:47 PM

Added new line.
Moved nativecodegen to the test.

mehdi_amini added inline comments.Jul 23 2023, 10:59 PM
mlir/include/mlir/Target/LLVM/ModuleToObject.h
29

That assumes that the operation can be translated to LLVM? This isn't clearly said here.

What about a SPIRV target?

fmorac marked 3 inline comments as done.Jul 24 2023, 4:30 AM
fmorac added inline comments.
mlir/include/mlir/Target/LLVM/ModuleToObject.h
29

I'll update the description.

SPIRV is out of scope for this class, that's also why we moved the class to Targets/LLVM.

mehdi_amini added inline comments.Jul 24 2023, 10:39 PM
mlir/include/mlir/Target/LLVM/ModuleToObject.h
29

Then shall it be in the mlir::LLVM namespace as well? The name is quite generic and could conflict.

fmorac updated this revision to Diff 544469.Jul 26 2023, 12:13 PM

Moved ModuleToObject into the namespace mlir::LLVM.

fmorac marked 2 inline comments as done.Jul 26 2023, 12:14 PM
fmorac updated this revision to Diff 545260.Jul 28 2023, 1:27 PM
fmorac retitled this revision from [mlir][ExecutionEngine] Adds an utility class for serializing operations to binary strings. to [mlir][Target][LLVM] Adds an utility class for serializing operations to binary strings..

Updating commit message.

fmorac updated this revision to Diff 545487.Jul 30 2023, 5:14 PM

Change the name of the test.

mehdi_amini added inline comments.Jul 30 2023, 5:57 PM
mlir/unittests/Target/LLVM/CMakeLists.txt
8

You marked @krzysz00 comment above "done", but we still have nativecodegen here, I guess it may be needed because you need to generate "some sort of object file" here?

What happens to this unit-test if I build LLVM with only the NVPTX target enabled? Or in general without a native target?

13

We should just link the dialects we need here.

fmorac planned changes to this revision.Jul 30 2023, 6:18 PM
fmorac added inline comments.
mlir/unittests/Target/LLVM/CMakeLists.txt
8

nativecodegen is needed because I'm creating a target machine and attaching it to the module, it fails if there's not a valid target.

What happens to this unit-test if I build LLVM with only the NVPTX target enabled? Or in general without a native target?

I would assume it fails and I didn't even think of that because I based the test on one from ExecutionEngine, I think the solution is skipping the test when the native target is not built.

13

I'll change it.

fmorac requested review of this revision.Jul 30 2023, 6:19 PM

I'd like to see this working with just AMDGPU target enabled (and with X86 or the native target) off so I can use this in a library without needing to pull in extra targets

I'd like to see this working with just AMDGPU target enabled (and with X86 or the native target) off so I can use this in a library without needing to pull in extra targets

The only component that needs nativecodegen is the unit test, there are no dependencies in nativecodegen on the MLIRTargetLLVM library, the class compiles and works with any target. See the unit test in D154117 in there I work with NVPTX, I plan to add a similar test for AMDGPU.

Having gone back and re-read, yeah, the only thing here is turning the test off when native codegen isn't available

fmorac updated this revision to Diff 546178.Aug 1 2023, 12:19 PM

Removed unnecessary libs & skip the test if the Native target is not being built.

mehdi_amini accepted this revision.Aug 1 2023, 12:43 PM
This revision is now accepted and ready to land.Aug 1 2023, 12:43 PM
mehdi_amini added inline comments.Aug 1 2023, 12:44 PM
mlir/unittests/Target/LLVM/SerializeToLLVMBitcode.cpp
38 ↗(On Diff #546178)

Does this build without the native target? (I assumed you tried?)

Can we make it not a global and instead done in the constructor of a fixture?

fmorac updated this revision to Diff 546196.Aug 1 2023, 1:17 PM
fmorac marked an inline comment as done.

Switched to Fixtures for the test.

mlir/unittests/Target/LLVM/SerializeToLLVMBitcode.cpp
38 ↗(On Diff #546178)

Yeah I tried with only NVPTX, those functions are always defined but have empty bodies if the target is not built, see https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/TargetSelect.h#L119

Done.

fmorac updated this revision to Diff 547727.Aug 7 2023, 5:09 AM

Rebasing.