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

[Lei Zhang via Phabricator via llvm-commits]

antiagainst added a comment.

In D72533#1865938 <https://reviews.llvm.org/D72533#1865938>, @bondhugula wrote:

> For the naming part, could we consider using IntegerType for signless integers the way it is now and add SignedIntegerType? i.e., see "Signed" as an addition as opposed to an adjective/specialization, and continuing to maintain that integer types in MLIR are signless.
>
> A couple of things.
>
> 1. The commit message has no mention of 'SignedIntegerType' or 'SignlessIntegerType'. Please add something to the effect "add 'SignedIntegerType' and IntegerType -> SignlessIntegerType".
> 2. The last message on https://groups.google.com/a/tensorflow.org/d/msg/mlir/XmkV8HOPWpo/7O4X0Nb_AQAJ was in Jul 2019. Could you mention the naming change on the discussion forum?


Functionality-wise it might not matter that much but engineering-wise, IMO, the current way is better because:

- Signedness is just one of the semantics bit belonging to integers, like bit width. So I think the current way it's conceptually clearer.
- Having different subclasses, say, `SignlessIntegerType`, `SignedIntegerType`, and `UnSignedIntegerType` (IMO we cannot just have  `SignlessIntegerType` and `SignedIntegerType` because using `SignedIntegerType` to mean both signed and unsigned integers can be quite confusing) we are basically duplicating the same set of utilities for integers three times. And one still would like to have an intermediate base integer class for cases where one don't care about signedness so ends up we are having the same set of utilities four times. We may be able to deduplicate and simplify with templating and other ways, but still it's complexity coming with the class hierarchy.

:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72533