[Mlir-commits] [mlir] [mlir-python] Emit only dialect `EnumAttr` registrations. (PR #117918)

[Jacques Pienaar]

jpienaar wrote:

> > The other is also that these should be prefixed with dialect. Problem 2 is resolved by requiring folks to prefix with namespace as is done most places.
> 
> Yes but it's upstream where the problem lies - there are many attributes that are in builtin and aren't prefixed. Such as all these in [linalg](https://github.com/llvm/llvm-project/blob/e7d09cecc9123f89ace1712a617e252d78b179e9/mlir/include/mlir/Dialect/Linalg/IR/LinalgEnums.td#L19) and [FastMathFlags](https://github.com/llvm/llvm-project/blob/4a7b56e6e7dd0f83c379ad06b6e81450bc691ba6/mlir/include/mlir/Dialect/Arith/IR/ArithBase.td#L118) in arith and the particular `IteratorType` offender here.

Yes I know Linalg is a offender here: I had made a lint check and started changing these a while ago. It's not that many that needs changes. The one where it was a bit more work is updating a generator generator here which relief more on names (it required introducing same behavior as we do for op definitions though, which is a win). Built-in being special is known, it has no prefix, that is known and also why we keep it small. The FastMathFlags one I easily addressed, but it ends up a bit confusing as it results in looking like a dialect dependency as it is intentionally reused elsewhere, but it doesn't affect the C++ side or dependencies, so in practice wasn't too much hard.

> 
> It actually wouldn't solve the problem anyway because of how the generation logic currently works:
> 
> https://github.com/llvm/llvm-project/blob/main/mlir/test/mlir-tblgen/enums-python-bindings.td#L99-L105
> 
> ```
> def TestBitEnum
>     : I32BitEnumAttr<"TestBitEnum", "", [
>         I32BitEnumAttrCaseBit<"User", 0, "user">,
>         I32BitEnumAttrCaseBit<"Group", 1, "group">,
>         I32BitEnumAttrCaseBit<"Other", 2, "other">,
>       ]> {
>   let genSpecializedAttr = 0;
>   let separator = " | ";
> }
> 
> def TestBitEnum_Attr : EnumAttr<Test_Dialect, TestBitEnum, "testbitenum">;
> 
> // CHECK: @register_attribute_builder("TestBitEnum")
> // CHECK: def _testbitenum(x, context):
> // CHECK:     return _ods_ir.IntegerAttr.get(_ods_ir.IntegerType.get_signless(32, context=context), int(x))
> 
> // CHECK: @register_attribute_builder("TestBitEnum_Attr")
> // CHECK: def _testbitenum_attr(x, context):
> // CHECK:     return _ods_ir.Attribute.parse(f'#TestDialect<testbitenum {str(x)}>', context=context)
> ```
> 
> So the "builtin" will always be emitted every single time it's used as the `enumInfo` of an `EnumAttr` (because again, tablegen has no knowledge that it's already emitted the registration in some other file).

Didnt we resolve this behavior in another instance by looking at the main file? I'm pretty sure we made a change to type and attribute emission.to address this by only considering the main file and using that as source of truth as to what should be emitted. This may end up that one has to use a different top file, but it also fits that one emits for the file given as input.

https://github.com/llvm/llvm-project/pull/117918