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

Tue Mar 17 09:38:10 PDT 2020

mravishankar accepted this revision.
mravishankar marked 2 inline comments as done.
mravishankar added inline comments.
This revision is now accepted and ready to land.

================
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.
----------------
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.

================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp:285
+
+static Optional<Type> convertTensorType(const spirv::TargetEnv &targetEnv,
+                                        TensorType type) {
----------------
antiagainst wrote:
> mravishankar wrote:
> > I understand that this is maintaining functionality. I am actually not sure it is a good thing to lower from TensorType directly into SPIR-V type. SPIRV Array Type is not an SSA data-structure. It is a load/store data-structure. Please remove this. We should never have supported this in the first place.
> There is complication here. The type conversion was introduced for lowering constant tensors (https://reviews.llvm.org/D73022). In SPIR-V one creates composite (vector/matrix/array/struct) constants with `OpConstantComposite` to embed relative large constant values and use `OpCompositeExtract` and `OpCompositeInsert`. So array types are not only just used in the load/store way; they can also be used in an SSA way like what we do for vectors. 
> 
> I guess the real problem is that in some frontend dialect (like TensorFlow) all things are represented as tensors, even for scalars. So this forced the tensor abstraction down the stack sometimes, especially during every development stage. I would think it's better to actually turn some constant tensors into constant vectors in a pre-pass so it's cleaner.
> 
> Added some comments.
Forgot about constants. That make sense. Thanks for the explanation.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76244