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

Sat Feb 8 15:05:50 PST 2020

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

================
Comment at: mlir/include/mlir/IR/OpBase.td:312
 // Any integer type irrespective of its width.
 def AnyInteger : Type<CPred<"$_self.isa<IntegerType>()">, "integer">;

----------------
rriddle wrote:
> Can you go through and make sure all of the current usages of IntegerType use the signless variant? Or is this something planned for a followup?
Sure! Since you are asking here and I changed some places, I went another pass over `isa<IntergerType>` and `isIntegerType` use sites and converted as many as possible to use signless ones. Remaining ones basically are for i1, parsing and int elements attr storage. So it's should be quite thorough now; but if you can go another pass that would be awesome. Some APIs are also adjusted to include 'Signless' in the name to be explicit so downstream users can see an explicit compilation break. 

================
Comment at: mlir/lib/IR/Attributes.cpp:283
+int64_t IntegerAttr::getInt() const {
+  assert(!getImpl()->getType().isUnsignedInteger() &&
+         "integer signedness unsupported");
----------------
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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72533