<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>Issue</th>
<td>
<a href=https://github.com/llvm/llvm-project/issues/144005>144005</a>
</td>
</tr>
<tr>
<th>Summary</th>
<td>
[mlir][tablegen] Underlying signless integer storage for Enum Attributes is handled incorrectly
</td>
</tr>
<tr>
<th>Labels</th>
<td>
mlir
</td>
</tr>
<tr>
<th>Assignees</th>
<td>
</td>
</tr>
<tr>
<th>Reporter</th>
<td>
0xMihir
</td>
</tr>
</table>
<pre>
# Description
If we create an integer enum attribute with a case that has the MSB set, it will fail validation due to an overflow.
The following case illustrates the bug.
FIrst, we can define an enum in tablegen:
```tablegen
def I32Case5: I32EnumAttrCase<"case5", 5>;
def I32Case10: I32EnumAttrCase<"case10", 10>;
def I32CaseSignedMaxPlusOne: I32EnumAttrCase<"caseSignedMaxPlusOne", 2147483648>;
def I32CaseUnsignedMax: I32EnumAttrCase<"caseUnsignedMax", 4294967295>;
def SomeI32Enum: I32EnumAttr<
"SomeI32Enum", "", [I32Case5, I32Case10,
I32CaseSignedMaxPlusOne, I32CaseUnsignedMax]>;
def I32EnumAttrOp : TEST_Op<"i32_enum_attr"> {
let arguments = (ins SomeI32Enum:$attr);
let results = (outs I32:$val);
}
```
Then, using the defined Op, we can observe that the last two cases fail:
```mlir
// CHECK-LABEL: func @allowed_cases_pass
func.func @allowed_cases_pass() {
// CHECK: test.i32_enum_attr
%0 = "test.i32_enum_attr"() {attr = 5: i32} : () -> i32
// CHECK: test.i32_enum_attr
%1 = "test.i32_enum_attr"() {attr = 10: i32} : () -> i32
// CHECK: test.i32_enum_attr
%2 = "test.i32_enum_attr"() {attr = 2147483648: i32} : () -> i32
// CHECK: test.i32_enum_attr
%3 = "test.i32_enum_attr"() {attr = 4294967295: i32} : () -> i32
return
}
```
This is because the underlying generator ([EnumsGen.cpp](https://github.com/llvm/llvm-project/blob/main/mlir/tools/mlir-tblgen/EnumsGen.cpp#L651)) and tablegen code ([EnumAttr.td](https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/EnumAttr.td#L43)) use the deprecated `getInt` method, which sign extends the last bit. I don't think this is the best default behavior, but I don't think that we should change this old API.
# Fix
To fix this, we can return `ZExtValue` inside EnumsGen.cpp like so:
```diff
diff --git a/mlir/tools/mlir-tblgen/EnumsGen.cpp b/mlir/tools/mlir-tblgen/EnumsGen.cpp
index 95767a29b9c3..def322a9d684 100644
--- a/mlir/tools/mlir-tblgen/EnumsGen.cpp
+++ b/mlir/tools/mlir-tblgen/EnumsGen.cpp
@@ -524,7 +524,7 @@ static void emitSpecializedAttrDef(const Record &enumDef, raw_ostream &os) {
os << formatv("{0} {1}::getValue() const {{\n", enumName, attrClassName);
- os << formatv(" return static_cast<{0}>(::mlir::IntegerAttr::getInt());\n",
+ os << formatv(" return static_cast<{0}>(::mlir::IntegerAttr::getValue().getZExtValue());\n",
enumName);
os << "}\n";
```
Similarly, for EnumAttr.td, we can use `APInt`'s `eq` method.
```diff
diff --git a/mlir/include/mlir/IR/EnumAttr.td b/mlir/include/mlir/IR/EnumAttr.td
index 9fec28f03ec2..8d004f8b7b8c 100644
--- a/mlir/include/mlir/IR/EnumAttr.td
+++ b/mlir/include/mlir/IR/EnumAttr.td
@@ -34,7 +34,7 @@ class IntEnumAttrCaseBase<I intType, string sym, string strVal, int intVal> :
EnumAttrCaseInfo<sym, intVal, strVal>,
SignlessIntegerAttrBase<intType, "case " # strVal> {
let predicate =
- CPred<"::llvm::cast<::mlir::IntegerAttr>($_self).getInt() == " # intVal>;
+ CPred<"::llvm::cast<::mlir::IntegerAttr>($_self).getValue().eq(APInt(" # intType.bitwidth # ", " # intVal # "))">;
}
// Cases of integer enum attributes with a specific type. By default, the string
```
Happy to open a PR for the above changes and additional test cases.
</pre>
<img width="1" height="1" alt="" src="http://email.email.llvm.org/o/eJy0WEtz2zgS_jXQpUsqCiT1OOigh72j2mTjij1z2IsLJJoSdiBAA4Cyvb9-qwFKoid2EldlXUpIgv1Co78PDQrv1c4gLli5YuVmINqwt26RPX9We-UGlZUvC8Zz2KCvnToGZQ3LltsGnhBqhyIgCAPKBNyhAzTtAUQITlVtQHhSYQ8CauERwl4E2AsPYY_w-X4FHgPja1ABnpTW0Ail4SS0koKcgGwRgiXj9oSu0fZpBCxbsmz5sEdorNb2SZldMq60bn1wImCyX7W7URK-3Tof_VC8woDERpkYc4xVGQii0rhDw3KSZ5Ms_S7D2VJiA9ucr4XHkuVLoIcb0x6WITgaZPmacV7H15yTs5LlNyxfvdYdZ6T8ru4465RJ7lvte1om-Vk83-nWfzH4XVvfCCfLfFxMi1k-KWZvefjd-LPad4335ZLdgs-L-WTK59d5px9Zv7cH7Gz9zSyZzJYAjPO-ULIZL-muXF2yz9e9bNI70n_r772sXQ30p1Fu-oF3OTmH-eUIFPjDzf3D45djyoPK-SMV0CMVO8WZ3wCbkj5oDCDcrj2gCR5YvgHGZ8r4vyWC8SLpzpNfiIoOfauvarYNnkJJ8iehL-JsuulX6wUZhmbYekIGASGVu4Qvxx4GbOXRnTpIkpQWPkB4shFMPkKxQ0PPxUErFxHIbxm_hfVvN-t_Dj8tVzefKDtNa2pgRSYIlygfo6XHo_CeZUt6OfqOBJ8xPj_nD_oOyHRAH0avE57EyqzLE39DhGrnbJYGomiEr8o5m27imnYiQ1o-Gv6Y-_FH3ScG-GX--Uf99_H_C-PIPxpHny9-GIfD0DrzXsnHqlcelIcKa9HGnQahNRKdfiEY7NCgE8G6aL1cEf78P9CM6uORgM9n-xCOPkKMZrxTYd9Wo9oeGL_V-nS-DI_O_gfrwPhtpW3F-O1BKEMXrRzjt8Fa7bvHYag07R389pU3nn-alGPCMJ-DMPKy90BtJfbiI9oZBfkrw1Om1q3E68D2axfe2RnPPxV5F9w5jxKPDmsRUAKbZDsMWxPYJIMDhr2VkVP2qt4DUSngc0Aj_ZVSKhVGsAVpDeNTohpl_qT_43LFfRp9II4SrQ5Q4V6clHVktWrDG4oiEIX5vW21hHovzA6TOaslLO-25w4hslQOt-qZBh4sNOo5CvZIMJUVzerfN8_hD6FbpIkp45VE6C8baPUngrffUqJUTUP7hWoaGA53KoD4SD1A9aHqyZbKSHyGeTmdTAWfV_M6H40kNjnnYi4nswLGWTYpCpYth8Phx2KJKVul34fjYkXGigyGJS8YX0-B8dXlNr3yQQRVw8kqCXhQ4f6ItRJa_Rclld8GG8ZntTU-wFesrZPA-IRYJL5ZgxNPj9YHh-JAb6zvbRipB7C0aa5ZvobGuoMIp8glnE1XWWSX6WpMBJIvWb7cYbfgiW6SX7I2XbFybbq-g9z_Sxxiz0DUtdbC-zRw3rMp0-95PjNXN3fa7wLJpYCo3-CzFE5Mdbzbpj46NUddpIS4GGfn9hJgWrJ3Z_6L3PcSNdphuILlvZh6eeul6dUKxXXZnLVSP_Oa1u_VQWnh9Aslv7EOXhHVBcREU2ySLe8SLTE-9fSMf1056sIJPwXaH7JkHxs_ptQLZBus-azJcqz5aDSTWVY0s2pazep3Ifszxt9E7E8pdoDNL3jNX8G1plKHrQn9E8AqnQK2dNp7eDlGXPjgaI_1L4f-U3B_UKu6Jkn6R0_UIEcChb7NrWksy9edfieaLCWlrqboj3p5jd73qrQLqRdQd0ahK9AecDF0aS9jn310KBXta9SPRBSv7xzK1Nyn2o97a7w7Q-d7aIlw4sWjR910ULkAN_pIHVKM6ZKRrvT56v_gvY9b_IvxWQJJooYuCkraqFLhScmwj4PXw1cv0uubiPd42jkjO_ZkEeDnfjEeIWzzzjcBf_4o4GkHaFQNgYKA1cu5DyD_1BukYvqWGn4Tx-MLBAv2iAYE3H2NBEEqorIn7BoDHxssIaUKyhqhY_-aDjgjGMhFLuf5XAxwMZ4W8-m0LLNisF9gWcyaYpzNZV01ZdbMq3E54_VUltgU83I-UAue8TKbjPMsK8qsGGVNPp_OeDYpUUzEHFmR4UEoPaIVHFm3GyjvW1yMiyLLyoEWFWofv7ZwngDKWbkZuEVs4qp251mRaeWDv1oIKuj4hSYqlBtWri6fJ8oN_H5tdn2HkUv2fbBO7PDCobC8roTysBdGapSgTG2dwzrol0Hr9OLDPWecJDUK3TxPC_6_AAAA___0E3jG">