[PATCH] D72765: [mlir][spirv] Support implied extensions and capabilities

[Lei Zhang via Phabricator via llvm-commits]

antiagainst added inline comments.


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h:53
+/// third one will also be returned.
+SmallVector<Capability, 0> getRecursiveImpliedCapabilities(Capability cap);
 
----------------
benvanik wrote:
> nit: wouldn't this just be std::vector?
According to LLVM programmer's manual, `SmallVector<..., 0>` has advantages over `std::vector`: http://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVTypes.cpp:89
+    for (Capability c : getDirectImpliedCapabilities(allCaps[i]))
+      if (!llvm::is_contained(allCaps, c))
+        allCaps.push_back(c);
----------------
rriddle wrote:
> I would imagine this would be much simpler/efficient if you used a SetVector instead.
Indeed. Thanks for the suggestion! :)


================
Comment at: mlir/tools/mlir-tblgen/SPIRVUtilsGen.cpp:1306
+
+    SmallVector<std::string, 1> impliedCaps;
+    std::vector<Record *> impliedCapsDefs = def.getValueAsListOfDefs("implies");
----------------
rriddle wrote:
> This seems unnecessarily inefficient. Can you just split the format below and use interleaveComma over the defs instead.
Good point! Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72765