[PATCH] D75611: [mlir][spirv] Support integer signedness

Wed Mar 4 12:21:39 PST 2020

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

================
Comment at: mlir/docs/Dialects/SPIR-V.md:261
+dialects. However, SPIR-V spec only defines two signedness semantics state: 0
+indicates unsigned, or no signedness semantics, 1 indicates signed semantics. So
+both `iN` and `uiN` are serialized into the same `OpTypeInt N 0`. For
----------------
mravishankar wrote:
> Do we need no signedness semantics. This is effectively saying no signedness is same as unsigned. Might as well make that explicit?
We still need it. They are serving different APIs: for Vulkan generally we want signless (but it's complicated. There are cases where one still want signedness semantics like image sampling stuff; it's due to historical reasons that high-level programming language was directly sent to the drivers). For OpenCL signedness is something they care. 

================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVBase.td:2948

+class IOrUI<int width>
+    : Type<Or<[CPred<"$_self.isSignlessInteger(" # width # ")">,
----------------
mravishankar wrote:
> Just to make sure I understand this, the presence of "signless" is effectively legacy. We are treating them as unsigned implicitly. 
Not really. We still want signless. It's just that the spec serializes both of them into one state... (I don't have the context as for why that decision instead of using three numbers...)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75611