[PATCH] D72533: [mlir] Add a signedness semantics bit to IntegerType

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.
Herald added a reviewer: herhut.


================
Comment at: mlir/include/mlir/Dialect/VectorOps/VectorOps.td:375
                  TCresIsSameAsOpBase<0, 1>>]>,
-     Arguments<(ins AnyType:$source, AnyVector:$dest, AnyInteger:$position)>,
+     Arguments<(ins AnyType:$source, AnyVector:$dest, AnySignlessInteger:$position)>,
      Results<(outs AnyVector)> {
----------------
Seems like some of these need to be formatted now.


================
Comment at: mlir/lib/IR/Attributes.cpp:283
+int64_t IntegerAttr::getInt() const {
+  assert(!getImpl()->getType().isUnsignedInteger() &&
+         "integer signedness unsupported");
----------------
antiagainst wrote:
> jpienaar wrote:
> > antiagainst wrote:
> > > rriddle wrote:
> > > > This is a slightly concerning. Is your plan to have getSInt/getUInt? Or remove this all together?
> > > I don't plan to add getSInt/getUInt at the moment because there is a TODO for removing this API entirely. I'm not sure whether that's still planned? @jpienaar 
> > Which TODO are you referring to?
> Here it is: https://github.com/llvm/llvm-project/blob/835c81923efee0cef1c64b25a34cf0872fa1e634/mlir/include/mlir/IR/Attributes.h#L355
I'd say that we could add them and just let the user decide which one to call. I'd be fine with doing what llvm::ConstantInt does, i.e. have a `getSExtValue` and `getZExtValue`.


================
Comment at: mlir/lib/IR/TypeDetail.h:59
   /// The hash key used for uniquing.
-  using KeyTy = unsigned;
-  bool operator==(const KeyTy &key) const { return key == width; }
+  using KeyTy = std::pair<unsigned, IntegerType::SignednessSemantics>;
+
----------------
I think we can keep the KeyTy the same(i.e. unsigned) and just construct it from unsigned+signedness when necessary. That would simplify a lot of this.


================
Comment at: mlir/lib/IR/TypeDetail.h:79
+  static unsigned
+  packWithAndSignedness(unsigned width,
+                        IntegerType::SignednessSemantics signedness) {
----------------
Can we just use a packed struct instead and convert between that and unsigned when necessary? That would remove the bit magic and make it easier to manipulate. e.g.

```
struct KeyBits {
  unsigned width : 30;
  unsigned signedness : 2;
};

KeyBits getKeyBits(unsigned value) {
  KeyBits asBits;
  memcpy(&asBits, &valueData, sizeof(asBits));
  return asBits;
}
unsigned getFromKeyBits(KeyBits) {
  // Reverse of above
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72533