[PATCH] D75875: [mlir][spirv] Support querying type extension/capability requirements

[Lei Zhang via Phabricator via llvm-commits]

antiagainst marked 10 inline comments as done.
antiagainst added inline comments.


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVTypes.cpp:583
+    LLVM_FALLTHROUGH;
+  case StorageClass::Input:
+  case StorageClass::Output:
----------------
mravishankar wrote:
> So input/output cant be 8 bit?
No. SPV_KHR_8bit_storage does not support 8-bit on Input/Output. Input/Output is not useful for compute pipeline where you can have only one compute shader. It's more for graphics pipeline where you've many shader stages and they pass data from one to another with Input/Output.


================
Comment at: mlir/lib/Dialect/SPIRV/Transforms/UpdateVCEPass.cpp:60
+          << op->getName() << "' requires at least one extension in ["
+          << llvm::join(extStrings, ", ")
+          << "] but none allowed in target environment";
----------------
rriddle wrote:
> nit: I don't think the join is actually necessary.
Hmm, I'm seeing compilation errors if missing it.


================
Comment at: mlir/lib/Dialect/SPIRV/Transforms/UpdateVCEPass.cpp:75
+/// convention.
+static LogicalResult checkAndUpdateCapabilityRequirements(
+    Operation *op,
----------------
mravishankar wrote:
> This and the above method are the same. Could just template this to reduce bloat
I've tried too. But we don't have a templated version of `stringifyEnum<EnumClass>` like what we have for `symbolizeEnum<EnumClass>`. I guess it's not really worth it for just two enum classes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75875