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

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added inline comments.
This revision now requires changes to proceed.
Herald added a reviewer: nicolasvasilache.


================
Comment at: mlir/include/mlir/IR/Builders.h:115
 
+  // Signed and unsigned integer attribute getters.
+  IntegerAttr getSI32IntegerAttr(int32_t value);
----------------
//// -> /////


================
Comment at: mlir/include/mlir/IR/StandardTypes.h:87
 
+  // Signedness semantics.
+  enum SignednessSemantics {
----------------
Use triple tick comments.


================
Comment at: mlir/lib/IR/Attributes.cpp:283
+int64_t IntegerAttr::getInt() const {
+  assert(!getImpl()->getType().isUnsignedInteger() &&
+         "integer signedness unsupported");
----------------
This is a slightly concerning. Is your plan to have getSInt/getUInt? Or remove this all together?


================
Comment at: mlir/lib/IR/Attributes.cpp:696
+  // Make sure signedness semantics is consistent.
+  switch (intType.getSignedness()) {
+  case IntegerType::Signed:
----------------
Can you tighten this up a bit?

if (intType.isSignless())
  return true;
return intType.isSigned() ? isSigned : !isSigned;


================
Comment at: mlir/lib/IR/MLIRContext.cpp:498
+                     MLIRContext *context) {
+  if (signedness == IntegerType::Signless) {
+    switch (width) {
----------------
Can you just switch this to early exit and leave the rest as is?


================
Comment at: mlir/lib/IR/TypeDetail.h:71
 
-  unsigned width;
+  IntegerType::SignednessSemantics signedness;
 };
----------------
Can you bit pack the signedness with the width?


================
Comment at: mlir/lib/IR/TypeDetail.h:330
+namespace llvm {
+template <>
+struct DenseMapInfo<::mlir::IntegerType::SignednessSemantics> {
----------------
You don't need all of this, you shouldn't need any of this if you follow the above. If it's still necessary, just implement a custom hashKey function for the integer type storage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72533