[Mlir-commits] [flang] [mlir] [mlir][AsmParser] Disambiguate location attributes from trailing loc (PR #180668)

[Jacques Pienaar]

jpienaar wrote:

Just given timezones of all involved, let me expand a bit on my question. I'm not convinced I see as much value in changing the syntax for the case where we have operations with optional attribute as the last part of their syntax (e.g., no type, no other syntax). This is, as, for IR format, one chooses a format that enables local debugging, which means in 95% of the cases you are printing the type. And probably in 90% of the remaining 5% cases you aren't using a location attribute. It is for me a very small domain which benefit from this change, but it ends up creating a syntax that overlaps with alias syntax and requires changing aliases everywhere.

Here one could have just as well done `test.optional_attr_op {loc(...)}` (e.g., require optional `{`,`}` for such an optional attribute if one wanted to avoid a AttrDict). Of course folks can innovate with things like `@` and the like too. But I'd even argue AttrDict isn't bad here, especially from readability point of view `test.optional_attr_op {tag = loc(...)}` (more options if one doesn't have only a singular attribute).

I'd rather flag them (https://github.com/llvm/llvm-project/compare/main...jpienaar:llvm-project:flagambig?expand=1 ) and document the potential issue, than changing syntax here. We have precedent for flagging in formatgen already. I checked this more restricted checked over a couple of dozen projects and didn't find a failure. And where there is a failure, the change is most likely less than the alias change.

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