[PATCH] D76244: [mlir][spirv] Make SPIRVTypeConverter target environment aware

Wed Mar 18 06:30:28 PDT 2020

antiagainst marked an inline comment as done.
antiagainst added inline comments.

================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVLowering.h:38
+///
+/// TODO(antiagainst): We might want to introduce a way to control how
+/// unsupported bitwidth are handled and explicitly fail if wanted.
----------------
mravishankar wrote:
> antiagainst wrote:
> > mravishankar wrote:
> > > Cant we do this right now? If any tests failed we can update the extension/capability accordingly and not just force expand right?
> > I'd prefer to defer this given it's not needed immediately and this commit is already ~1000 LOC change. The TODO here is not meant for tests. For tests where we care about the type conversions, we should probably test both with and without capabilities. For tests where we don't care about type conversions, it does not matter. This is for real-world use cases. Given a hardware implementation, the extension/capabilities are already predetermined; adding more does not work and we must dance inside the boundaries here. I'd assume this is the majority of the use cases.
> Sorry I wasnt being clear. I meant that for tests that fail we add the extension/capabilities needed. For the "real-world" use case where the extension/capability comes from the hardware, the type conversion should fail (and not force expand). But its fine to force expand for now and revisit later.
Yeah, agreed. I think a more appropriate approach is we emulate the support for shorter bitwidth if the target hardware does not support it directly. For example, if the target hardware does not support 16bit values in StorageBuffer, instead of forcing the runtime to cast all 16bit values from buffer A into 32-bit values into another buffer B and pass buffer B to the shader, we should do the magic in the compiler to treat every 32-bit value as two 16-bit values and use bitmasks to load/store and adjust offsets so that the runtime does not need to do the cast and we can save memory bandwidth. But you can see this is quite complicated. I'll look into supporting that later.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76244/new/

https://reviews.llvm.org/D76244