Page MenuHomePhabricator

[mlir][spirv] Create separate selection header block during serialization
ClosedPublic

Authored by antiagainst on Dec 11 2021, 11:35 AM.

Details

Summary

The previous "optimization" that tries to reuse existing block for
selection header block can be problematic for deserialization
because it effectively pulls in previous ops in the selection op's
enclosing block into the selection op's header. When deserializing,
those ops will be placed in the selection op's region. If any of
the previous ops has usage after the section, it will break. That
is, the following IR cannot round trip:

mlir
^bb:
  %def = ...
  spv.mlir.selection { ... }
  %use = spv.SomeOp %def

This commit removes the "optimization" to always create new blocks
for the selection header.

Depends On D115560

Diff Detail

Event Timeline

antiagainst created this revision.Dec 11 2021, 11:35 AM
antiagainst requested review of this revision.Dec 11 2021, 11:35 AM
Hardcode84 accepted this revision.EditedDec 11 2021, 2:37 PM

Was this issue about serializer generating completely invalid blob or valid blob we just can't deserialize (yet?)?
In any case, does it make sense to add sanity check on deserializer side to at least fail gracefully on such blob?

This revision is now accepted and ready to land.Dec 11 2021, 2:37 PM

Add explicit check and improve error reporting

Was this issue about serializer generating completely invalid blob or valid blob we just can't deserialize (yet?)?

It's more of the the second case. The generated blob is valid for this case per se; although I suspect it can be problematic for other cases by just reusing the existing block for the header block (and it's bad to not round trip anyway) so I fixed the serialization here.

We cannot structurize the control flow for this case (which can also be generated by some other SPIR-V generator) during deserialization because right now we try to "sink" all blocks in the selection construct into a spv.mlir.selection op. If the header block contains a value that are used after the newly formed selection op, then it won't work. It's related to the fact that spv.mlir.selection and spv.mlir.loop do not support yield semantics so values in their region blocks cannot "escape" and have uses outside. (We could consider supporting yielding; but it's a much larger fight that I'd prefer to leave it later. We could also inspect the uses and try to not sink those ones with outside uses but that's nasty again, considering phi values, etc.)

Fundamentally deserialization is solving a harder problem than serialization, given that deserialization is sort of raising the abstraction levels by going from plain basic blocks into regions. Also consider the possible SPIR-V blobs in the wild, the surface area is also huge. So making deserialization robust would require quite some investment. Thus far we have been trying to at least round trip, which is a way to make sure it's not lagging behind too much there (in the hope of eventually we can import in SPIR-V blobs from other generators freely!). Serialization matters more if speaking of compilation from a high level input though. It is lowering so it's easier. It is already relatively robust right now.

In any case, does it make sense to add sanity check on deserializer side to at least fail gracefully on such blob?

Yup. I've added an explicit check for this case and turned some other asserts into proper errors.

Hardcode84 accepted this revision.Dec 12 2021, 10:13 AM

Thanks for the explanation

mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp
1806

nit: !op.use_empty() ?

This revision was landed with ongoing or failed builds.Dec 13 2021, 7:42 AM
This revision was automatically updated to reflect the committed changes.
antiagainst marked an inline comment as done.Dec 13 2021, 7:43 AM