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

[Lei Zhang via Phabricator via llvm-commits]

antiagainst marked 6 inline comments as done.
antiagainst added inline comments.
Herald added a subscriber: bader.


================
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>;
+
----------------
rriddle wrote:
> 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.
I tried but wasn't able to get this working properly. Anway this is an internal thing that we can improve later. So leaving it as-is for now.


================
Comment at: mlir/lib/IR/TypeDetail.h:79
+  static unsigned
+  packWithAndSignedness(unsigned width,
+                        IntegerType::SignednessSemantics signedness) {
----------------
rriddle wrote:
> 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
> }
> ```
Thanks! I used `KeyBits` to simplify a few things.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72533