Page MenuHomePhabricator

[MLIR][SPIRV] Start module combiner.
ClosedPublic

Authored by ergawy on Oct 23 2020, 3:50 AM.

Details

Summary

This commit adds a new library that merges/combines a number of spv
modules into a combined one. The library has a single entry point:
combine(...).

To combine a number of MLIR spv modules, we move all the module-level ops
from all the input modules into one big combined module. To that end, the
combination process can proceed in 2 phases:

(1) resolving conflicts between pairs of ops from different modules
(2) deduplicate equivalent ops/sub-ops in the merged module. (TODO)

This patch implements only the first phase.

Diff Detail

Event Timeline

ergawy created this revision.Oct 23 2020, 3:50 AM
ergawy requested review of this revision.Oct 23 2020, 3:50 AM
ergawy added inline comments.Oct 23 2020, 4:03 AM
mlir/include/mlir/Dialect/SPIRV/ModuleCombiner.h
2

Oh, forgot to add copyright headers and such. Will do.

ergawy updated this revision to Diff 300284.Oct 23 2020, 7:15 AM
  • Add copyright headers.
ergawy updated this revision to Diff 300301.Oct 23 2020, 8:10 AM
  • Remove trivial braces.
mravishankar resigned from this revision.Oct 26 2020, 10:32 AM
antiagainst requested changes to this revision.Oct 26 2020, 1:45 PM

This looks awesome! I love the extensive tests! :)

mlir/include/mlir/Dialect/SPIRV/ModuleCombiner.h
25

s/spv/SPIR-V/

We should prefer to use the formal "SPIR-V' in comments as we are not restricted by valid symbol characters in comments. :)

29

I'd use

resolve ... / deduplicate ...
or
resolving ... / deduplicating ...

for consistency.

35

We might be able to condense this comment to just saying something like

  • If both of the two symbols are for spv.func, then ..
  • One of the two symbols is for spv.func. then ..
  • None of the two symbols is for spv.func. then ..

?

85

Could you also document what happened to the input modules (left as untouched)?

86

We can use SmallVectorImpl here to avoid forcing the caller to use 4.

86

This should return a bool to indicate whether it is successful ? Actually what about just return the newly combined module op (so the caller can perform additional work on it without explicitly rediscovering it again from combinedModuleBuilder). Return nullptr to indicate error?

87

We should also have a way to let the callers know what symbols is mapped to what? How about registering a llvm::function_ref listener here? Signature can be (spirv::ModuleOp sourceOp, StringRef oldSymbol) -> StringRef newSymbol.

mlir/lib/Dialect/SPIRV/Linking/ModuleCombiner/ModuleCombiner.cpp
22

Please avoid using namespace spirv as we can pull in symbols of the same name (like ModuleOp, etc.). (We are using it in the codebase but it should be fixed actually.)

25

s/nextConflictID/nextFreeID/ or something like that? nextConflictID reads to me that it's an ID that triggers confliction. :)

26

No need to use the const: in mlir ir constructs normally don't require const modifier. See https://mlir.llvm.org/docs/Rationale/UsageOfConst/

30

This appends the number to the symbol again (like _123456)? I'd assume we want to mutate the number suffix?

47

s/globalVarOp/op/ (given this is not for global variable specifically)

48

if (!...) return WalkResult::advance();

MLIR typically prefers early exit if possible.

55

return op.emitError(...) here. Also should we let the function to report an error properly? Silently ignoring the issue seems not good.

83

module.emitError. combinedModule is still something under construction.

90

What about going through all the symbols in the cloned module and renaming them in one pass? I think you can do something like

for (auto op: module.getBlock().without_terminator()) {
  if (auto symbolOp = dyn_cast<SymbolOpInterface>(op)) ...
}

This way we avoids multiple iterations.

122

You can use moduleClone.getBlock().without_terminator() to avoid checking the module end op.

mlir/test/lib/Dialect/SPIRV/TestModuleCombiner.cpp
37

No need to explicitly erase here?

This revision now requires changes to proceed.Oct 26 2020, 1:45 PM

Drive by comment: Why name this ModuleCombiner instead of ModuleLinker?

Drive by comment: Why name this ModuleCombiner instead of ModuleLinker?

Because it does not do the stuff related to what one would expect from a linker. It just merges the contents from multiple SPIR-V modules; no symbol resolution across modules, no weak/strong symbols, etc. These issues do not exist for Vulkan-flavored SPIR-V because each SPIR-V module is required to be standalone. But for OpenCL-flavored SPIR-V they do matter. I guess in the future this may evolve into that but until then keeping it clear seems preferable to me. :)

Drive by comment: Why name this ModuleCombiner instead of ModuleLinker?

Because it does not do the stuff related to what one would expect from a linker. It just merges the contents from multiple SPIR-V modules; no symbol resolution across modules, no weak/strong symbols, etc. These issues do not exist for Vulkan-flavored SPIR-V because each SPIR-V module is required to be standalone. But for OpenCL-flavored SPIR-V they do matter. I guess in the future this may evolve into that but until then keeping it clear seems preferable to me. :)

Thanks for the response Lei.

ergawy marked 17 inline comments as done.Oct 27 2020, 2:36 AM

@antiagainst thanks a lot for the review. Your comments simplify things by quite a bit.

mlir/include/mlir/Dialect/SPIRV/ModuleCombiner.h
87

I think the signature should rather be: (spirv::ModuleOp sourceOp, StringRef oldSymbol, StringRef newSymbol) -> void, right? This way the listener will have access to the new and old symbol as part of the callback invocation. Or did I miss your point?

Also, we need to properly test this at some point using GTest maybe! However, I prefer to add a TODO and continue with the rest of the functionality first if you don't mind. (I expect we would need some tiny infrastructure, e.g. mocking and building MLIR entities ourselves, in place in order to do so).

ergawy updated this revision to Diff 300923.Oct 27 2020, 2:38 AM
  • Handle review comments:
ergawy added inline comments.Oct 27 2020, 10:36 AM
mlir/include/mlir/Dialect/SPIRV/ModuleCombiner.h
87

One more thing, is it wise to pass 'StringRef' to those callbacks? I mean you never know whether the user-supplied code will outlive the scope that created the string underlying the ref.

Generally looks good to me! Just a few more nits. Also please make sure adding the listener.

mlir/include/mlir/Dialect/SPIRV/ModuleCombiner.h
50

Sorry actually ArrayRef<ModuleOp> here? :)

50

Should return OwningSPIRVModuleRef to handle ownership and avoid leaks.

87

You are right about the signature. It's fine to use StringRef; typically the caller should copy it if they want to retain. We can be explicit in the comment for the signature.

mlir/lib/Dialect/SPIRV/Linking/ModuleCombiner/ModuleCombiner.cpp
27

Have a limit over nextFreeID to avoid infinite loop? It's a bit arbitrary; but I'd assume 2^20 might be a good number for now.

28

Nit: by ++nextFreeID, it's actually lastUsedID here. ;-P So I'd expect nextFreeID++ here. (You can set the intial value of nextFreeID as 1 if wanting to be 1 based.)

31

can directly return possible?

93

s/dyn_cast/isa/

antiagainst requested changes to this revision.Oct 29 2020, 7:25 AM
This revision now requires changes to proceed.Oct 29 2020, 7:25 AM
ergawy updated this revision to Diff 301837.Fri, Oct 30, 2:29 AM
ergawy marked 7 inline comments as done.
  • Handle review comments.
mlir/include/mlir/Dialect/SPIRV/ModuleCombiner.h
50

Sorry actually ArrayRef<ModuleOp> here? :)

I had to use MutableArrayRef<ModuleOp> since the ModuleOp API is non-const. Let me know if I misunderstood something.

mlir/lib/Dialect/SPIRV/Linking/ModuleCombiner/ModuleCombiner.cpp
28

Indeed. I renamed to lastUsedID (laziest solution that works 😁 )

31

I feel better having a single exit point here 😁 . Please let me know if you have a strong opinion against this.

ergawy updated this revision to Diff 301855.Fri, Oct 30, 4:13 AM

Squash all commits into one.

antiagainst accepted this revision.Fri, Oct 30, 6:17 AM

Nice! Thanks @ergawy !

mlir/lib/Dialect/SPIRV/Linking/ModuleCombiner/ModuleCombiner.cpp
120

This can also be guarded by && symRenameListener?

This revision is now accepted and ready to land.Fri, Oct 30, 6:17 AM
ergawy added inline comments.Fri, Oct 30, 6:26 AM
mlir/lib/Dialect/SPIRV/Linking/ModuleCombiner/ModuleCombiner.cpp
120

Didn't do this to make sure the management of symNameToModuleMap is properly handled. I can also guard all code that manages the map with the same check but this is cleaner.

ergawy added inline comments.Fri, Oct 30, 6:36 AM
mlir/lib/Dialect/SPIRV/Linking/ModuleCombiner/ModuleCombiner.cpp
120

but _thought_ this _would be_ cleaner.

This revision was landed with ongoing or failed builds.Fri, Oct 30, 6:37 AM
This revision was automatically updated to reflect the committed changes.
antiagainst added inline comments.Fri, Oct 30, 6:41 AM
mlir/lib/Dialect/SPIRV/Linking/ModuleCombiner/ModuleCombiner.cpp
120

No worries! I changed it before landing, plus a few other tweaks. :)

But I don't have full visibility into what your planned next batch of code so I may very well misunderstand things. If that's the case, please just feel free to change it back. :)

Sorry I had to revert this back because it broke the build: https://buildkite.com/mlir/mlir-core/builds/8983#511574bd-3519-4f34-afd7-ef5702305815
Seems like a missing dependency somewhere.