[Mlir-commits] [mlir] [mlir][AsmParser] Fix parseOptionalAttribute consuming loc() as an attribute (PR #180668)

[llvmlistbot at llvm.org]

XiaoleiShi-NV wrote:

> Overall, I think this is going in the right direction. But there is now a discrepancy between `parseOptionalAttribute` and `parseAttribute`. If we remove `Token::kw_loc` from `parseOptionalAttribute`, we should also remove it from `parseAttribute`.

@matthias-springer  Thanks for the review! You raise a good point about the discrepancy.

Removing `Token::kw_loc` from `parseAttribute` as well would mean `loc(...)` is no longer parseable as an inline attribute value. This would affect:

- Attribute dictionary values: `{foo.loc_attr = loc(unknown)}`
- Required `LocationAttr` op arguments: `test.op_with_loc_attr loc("loc1":10:20)`
- Tests in `mlir/test/IR/locations.mlir` (lines 33-43, 110-112)

The printer side (`AsmPrinter.cpp:2553`) currently prints `LocationAttr` attribute values using `printLocation`, which outputs the `loc(...)` syntax. So removing from `parseAttribute` without updating the printer would break roundtripping.

Making this fully consistent would require changing how `LocationAttr` is printed/parsed when it appears as an attribute value -- likely through the dialect attribute mechanism instead of the `loc(...)` keyword syntax. That's a larger MLIR syntax change.

Would you prefer:
1. Expanding this PR to handle the full `parseAttribute` removal (plus printer changes and test updates), or
2. Keeping this PR scoped to the `parseOptionalAttribute` fix?

I'll address the test dialect feedback in the meantime.

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