Page MenuHomePhabricator

[MLIR][SPIRV] ModuleCombiner: deduplicate global vars, spec consts, and funcs.
ClosedPublic

Authored by ergawy on Nov 6 2020, 8:31 AM.

Details

Summary

This commit extends the functionality of the SPIR-V module combiner
library by adding new deduplication capabilities. In particular,
implementation of deduplication of global variables and specialization
constants, and functions is introduced.

For global variables, 2 variables are considered duplicate if they either
have the same descriptor set + binding or the same built_in attribute.

For specialization constants, 2 spec constants are considered duplicate if
they have the same spec_id attribute.

2 functions are deduplicated if they are identical. 2 functions are
identical if they have the same prototype, attributes, and body.

Diff Detail

Event Timeline

ergawy created this revision.Nov 6 2020, 8:31 AM
ergawy requested review of this revision.Nov 6 2020, 8:31 AM
ergawy added inline comments.Nov 6 2020, 8:35 AM
mlir/lib/Dialect/SPIRV/Linking/ModuleCombiner/ModuleCombiner.cpp
128

As mentioned already on an abandoned review (copied in order not to be lost):

This is a partial copy of BlockEquivalenceData from here: https://github.com/llvm/llvm-project/blob/474f7639e3494d9605f4444b087f48e710fbb0d4/mlir/lib/Transforms/Utils/RegionUtils.cpp#L374. I only copied the bare minimum needed for what I want to do in this patch.

However, I think it would be great if the original (not this stolen adaptation :) ) BlockEquivalenceData be put in some util header file accessible outside RegionUtils.cpp.

ergawy updated this revision to Diff 303818.Nov 9 2020, 4:01 AM

Some minor re-formatting.

antiagainst requested changes to this revision.Nov 12 2020, 7:43 AM

Looks awesome, thanks!

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

Computes

90

We also need to consider the type and other potential attributes here. (Actually type is represented as an attribute on the op. so essentially we need to look at all attributes except the name.)

109

Computes

114

Also need to consider type and other potential attributes

128

Is there a need to keep the BlockEquivalenceData structure? I think we can just make this as a computeHash function?

131

I think the default OperationEquivalence::computeHash does not consider regions? So it will be problematic for operations like spv.loop/etc. We probably need to enhance OperationEquivalence::computeHash to support regions. For now, feel free to directly return hash 0 for ops with regions. We can handle them in a following up patch.

135

Computes

234

SmallVector<.., 0>?

238

symbolOp = dyn_cast<SymbolOpInterface>(op);

mlir/test/Dialect/SPIRV/Linking/ModuleCombiner/deduplication_basic.mlir
50

This isn't correct. Two different entry points are basically two different kernels. So they are allowed to point to the same (set, binding) but with different types.

This revision now requires changes to proceed.Nov 12 2020, 7:43 AM
ergawy updated this revision to Diff 305155.Nov 13 2020, 8:11 AM

Handle review comments.

ergawy updated this revision to Diff 305164.Nov 13 2020, 8:25 AM
ergawy marked 9 inline comments as done.

Handle further comments.

Thanks for taking the time to review.

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

I left to bring attention to the fact that this is a partial copy of the original structure with the same name. To make things cleaner, I will do as you suggested and add a TODO to check if we can extract the original utility to some common header file.

mlir/test/Dialect/SPIRV/Linking/ModuleCombiner/deduplication_basic.mlir
50

I changed the implementation so that such situation maintains the 2 variables without producing an error. Just a side question, should we validate an spv module to make sure that the 2 variables are not referenced by call graph rooted at a any of the entry points? I think such situation should result in the module being invalid, right?

ergawy marked an inline comment as done.Nov 13 2020, 8:26 AM
antiagainst requested changes to this revision.Nov 18 2020, 8:01 AM

Awesome, just one final nit.

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

We should just directly return if the op contains regions?

mlir/test/Dialect/SPIRV/Linking/ModuleCombiner/deduplication_basic.mlir
50

Actually even for the some entry point, you can access two different global variables bound to the same (set, binding) pair. It's a way to alias. It's allowed.

This revision now requires changes to proceed.Nov 18 2020, 8:01 AM
ergawy updated this revision to Diff 306384.Nov 19 2020, 5:42 AM
  • Handle review comment.
  • Rebase.
ergawy marked an inline comment as done.Nov 19 2020, 5:42 AM
ergawy added inline comments.
mlir/lib/Dialect/SPIRV/Linking/ModuleCombiner/ModuleCombiner.cpp
107

Absolutely, if there are regions, with the current logic we might get false positives if 2 block match in all ops except for the region-containing ones.

ergawy updated this revision to Diff 306387.Nov 19 2020, 5:45 AM
ergawy marked an inline comment as done.
  • Add missing check in deduplication code.
antiagainst accepted this revision.Nov 19 2020, 7:03 AM

Awesome. Thanks a lot for this!

This revision is now accepted and ready to land.Nov 19 2020, 7:03 AM