core: Fix location in func block arguments by ZahidWakeel-synthara · Pull Request #5690 · xdslproject/xdsl

ZahidWakeel-synthara · 2026-03-02T08:29:13Z

func.func block arguments having locations info are not parsed in xDSL and hence throws parsing errors.

func.func @f_attrs_then_loc(%arg1 : i32 {foo.bar = 2 : i32} loc(unknown)) -> i32 {
      func.return %arg1 : i32
    }

xDSL raises parsing error because it doesn't expect location to be there inside func.func block arguments.

raise ParseError(at_position, msg)
xdsl.utils.exceptions.ParseError: parse_with_location.mlir:20:30
    func.func @f_attrs_then_loc(%arg1 : i32 {foo.bar = 2 : i32} loc(unknown)) -> i32 {
                             ^^^
                             ')' expected

while the same mlir code is parsed by mlir tools without any parsing errors. MLIR also expects the location to be in a specific order like %arg: <type> {attr} loc(...)
mlir supports this kind of location format for instance -

module {
  func.func @f(%arg0: i32 {foo.bar = 1 : i32} loc(unknown)) {
    func.return
  }
}

while if the order of location format is something like %arg: <type> loc(...) {attr} , then it is not supported in MLIR -

module {
  func.func @f(%arg0: i32 loc(unknown) {foo.bar = 1 : i32}) {
    func.return
  }
}

ZahidWakeel-synthara · 2026-03-02T08:30:57Z

@superlopuh @JosseVanDelm @math-fehr Please take a look at this, thanks.

codecov · 2026-03-02T08:37:44Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.34%. Comparing base (6c1c5a2) to head (3994a23).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5690      +/-   ##
==========================================
- Coverage   86.34%   86.34%   -0.01%     
==========================================
  Files         406      406              
  Lines       57723    57744      +21     
  Branches     6706     6710       +4     
==========================================
+ Hits        49843    49860      +17     
- Misses       6322     6325       +3     
- Partials     1558     1559       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

xdsl/dialects/utils/format.py

tests/test_parser.py

xdsl/dialects/utils/format.py

superlopuh · 2026-03-03T22:42:34Z

xdsl/dialects/utils/format.py

+            # If a location was parsed, reject attrs that appear after it,
+            # including empty dictionaries (e.g. `%arg: i32 loc(...) {}`).
+            if has_loc:
+                arg_attr_pos = parser.pos
+                parser.parse_optional_dictionary_attr_dict()
+                if parser.pos != arg_attr_pos:
+                    parser.raise_error(
+                        "Expected function argument attributes before location."
+                    )
+
+            # Reject duplicate locations (e.g. `%arg: i32 ... loc(...) loc(...)`).
+            if has_loc and parser.parse_optional_location() is not None:
+                parser.raise_error(
+                    "Expected at most one location in function argument."
+                )


Does MLIR have similar errors? I feel like this is a little overkill, as long as a parse error is raised for incorrect format that seems good enough to me, we can't realistically handle all combinations of wrong orders of elements of the IR.

@superlopuh I think we can remove the duplicate location check, but should definitely keep the attr before location check as MLIR strictly enforces it.

Sorry what I mean is that if this check is removed, it should still raise a parse error, right? Just with a different error message? For the same test.

@superlopuh Yes right, but the existing error message is not super clear.

Fair enough, let's keep the nicer message. I think we're safe testing it with just one test though, as opposed to the current empty and not-empty dictionary.

Previously the empty dict test case was failing, so I had to take care of parser pos before & after parsing attr dict. Hence I added the empty dict test case.

JosseVanDelm

Awesome! For me this pull request is good to go, if you address the following points:

Please add an extra check test to see if the location information is properly printed. You can do that within the current filecheck tests by using --check-prefix=CHECKPREFIX, see the inline suggestion.
Also there are quite a few other users of the print_func_op_like, adding tests for all of them is probably overkill, but could you please also add a test case (+interop test case) for at least one llvm.func operation?

Thank you so much!
Cheers!

JosseVanDelm · 2026-03-04T09:35:14Z

tests/filecheck/mlir-conversion/with-mlir/dialects/func/func_ops.mlir

@@ -1,2 +1,2 @@
 // RUN: xdsl-opt --print-op-generic %s | mlir-opt --mlir-print-op-generic --allow-unregistered-dialect | xdsl-opt | filecheck %s
 // RUN: xdsl-opt %s | mlir-opt --mlir-print-op-generic --allow-unregistered-dialect | xdsl-opt | filecheck %s


I would add an extra check here to actually check the print with debuginfo, something like:

// RUN: xdsl-opt %s --print-debuginfo | mlir-opt --mlir-print-op-generic --allow-unregistered-dialect | xdsl-opt | filecheck %s --check-prefix=DEBUGINFO

--check-prefix=DEBUGINFO allows you to add extra "check-like" lines like

// DEBUGINFO: check the debuginfo print here // DEBUGINFO-NEXT: check the debuginfo print on the next line here

@JosseVanDelm @superlopuh In the PR, currently locations attached with func block args are only parsed but not propagated, so applying xdsl-opt driver on such IR results in unknown locations.

builtin.module { func.func private @decl_only(%arg0: i32 loc("decl.mlir":1:1)) func.func @with_body(%arg0: i32 loc("body.mlir":2:3)) { func.return } }

On running with xdsl-opt --print-debuginfo

builtin.module { func.func private @decl_only(i32) -> () func.func @with_body(%arg0 : i32 loc(unknown)) { func.return } }

Function declarations behaviour is similar to how it behaves in MLIR, but function with body should preserve its arg locations which is not the case.
I have implemented a quick fix here: 1436496
Let me know if we can go ahead with this or not.

This looks great, and looks like it would be good to add as a separate PR before merging this one

@JosseVanDelm @superlopuh

Added this RUN command.
// RUN: xdsl-opt %s --print-debuginfo | mlir-opt --mlir-print-op-generic --allow-unregistered-dialect | xdsl-opt --print-debuginfo | filecheck %s --check-prefix=DEBUGINFO

I tried to mimic MLIR's behaviour as much as possible except one scenario.

func.func private @f_decl_unnamed_attr_then_loc(i32 {test.arg_name = "x"} loc("model.mlir":7:9)) // XDSL-CHECK: func.func private @f_decl_unnamed_attr_then_loc(i32) -> () // MLIR-CHECK: func.func private @f_decl_unnamed_attr_then_loc(i32 {test.arg_name = "x"})

Locations are dropped for declarative functions in MLIR, while in xDSL, both attributes & locations are dropped. I don't need this behaviour currently in xDSL hence didn't fix it as this required changes in multiple places.

If this sounds good, we can go for merge and I will push another PR for propagating locations for func block args. Currently known locations turn to unknown locations after parsing. See the func_ops.mlir tests for reference.

func.func private @f_named_attr_then_loc(%arg0: i32 {test.arg_name = "x"} loc("file.mlir":5:7)) { func.return } // DEBUGINFO: func.func private @f_named_attr_then_loc(%{{.*}} : i32 {test.arg_name = "x"} loc(unknown))

how much more code would it be to add it to this PR?

I think at this stage we should probably use this PR to reach the final state that we want to merge, and then either merge it all in one go or pull out independent changes, depending on how the code ends up looking.

JosseVanDelm · 2026-03-04T09:37:57Z

tests/filecheck/dialects/func/func_invalid.mlir

+
+func.func private @f(%arg0: i32 loc(unknown) {test.arg_name = "x"})
+
+// CHECK: Expected function argument attributes before location.


I get a different error message in MLIR:

➜ snax-mlir git:(main) ✗ mlir-opt test_loc.mlir test_loc.mlir:5:45: error: expected ')' func.func private @f(%arg0: i32 loc(unknown) {test.arg_name = "x"}) ^

But I like your implementation

JosseVanDelm · 2026-03-04T09:38:42Z

tests/filecheck/dialects/func/func_ops.mlir

@@ -1 +1 @@
 // RUN: XDSL_ROUNDTRIP


Please also add an extra line for debuginfo print testing here (see other comment in interop tests)

Since mlir-opt doesn't recognize xdsl based dialects like test, I get this error -

<stdin>:13:33: error: unknown type! func.func @arg_rec(%0 : !test.type<"int"> loc(unknown)) -> !test.type<"int"> {

Hence I added this RUN command:
// RUN: xdsl-opt %s --print-debuginfo | filecheck %s --check-prefix=DEBUGINFO

please add --allow-unregistered-dialect

Signed-off-by: Zahid Wakeel <zahid.wakeel@synthara.ai>

tests/test_printer.py

Signed-off-by: Zahid Wakeel <zahid.wakeel@synthara.ai>

ZahidWakeel-synthara marked this pull request as ready for review March 2, 2026 08:29

superlopuh reviewed Mar 2, 2026

View reviewed changes

xdsl/dialects/utils/format.py Outdated Show resolved Hide resolved

superlopuh reviewed Mar 2, 2026

View reviewed changes

tests/test_parser.py Outdated Show resolved Hide resolved

superlopuh reviewed Mar 2, 2026

View reviewed changes

tests/test_parser.py Outdated Show resolved Hide resolved

superlopuh reviewed Mar 2, 2026

View reviewed changes

tests/test_parser.py Outdated Show resolved Hide resolved

superlopuh reviewed Mar 3, 2026

View reviewed changes

xdsl/dialects/utils/format.py Outdated Show resolved Hide resolved

superlopuh reviewed Mar 3, 2026

View reviewed changes

ZahidWakeel-synthara force-pushed the zahidw/func_loc_order branch from 3dfe34b to dee68cf Compare March 4, 2026 00:05

JosseVanDelm self-requested a review March 4, 2026 09:05

superlopuh assigned ZahidWakeel-synthara Mar 4, 2026

superlopuh added the core xDSL core (ir, textual format, ...) label Mar 4, 2026

JosseVanDelm reviewed Mar 4, 2026

View reviewed changes

ZahidWakeel-synthara added 6 commits March 4, 2026 14:49

Fix func op args location order

a9ffa20

Signed-off-by: Zahid Wakeel <zahid.wakeel@synthara.ai>

Move tests to filecheck based tests

37274d0

Signed-off-by: Zahid Wakeel <zahid.wakeel@synthara.ai>

Print func.func block arg attributes before location

dda3805

Signed-off-by: Zahid Wakeel <zahid.wakeel@synthara.ai>

Refactor func block arg parsing logic

7db33a2

Signed-off-by: Zahid Wakeel <zahid.wakeel@synthara.ai>

Qualify dialect name for func arg attr in filecheck tests

8d98fe3

Signed-off-by: Zahid Wakeel <zahid.wakeel@synthara.ai>

Remove duplicate location check

0062b59

Signed-off-by: Zahid Wakeel <zahid.wakeel@synthara.ai>

superlopuh reviewed Mar 4, 2026

View reviewed changes

tests/test_printer.py Outdated Show resolved Hide resolved

ZahidWakeel-synthara added 3 commits March 5, 2026 19:45

Parse & consume attributes for func declarations (unnamed func args)

1a17074

Signed-off-by: Zahid Wakeel <zahid.wakeel@synthara.ai>

Update filecheck tests

ee44345

Signed-off-by: Zahid Wakeel <zahid.wakeel@synthara.ai>

Remove unnecessary printer test

180dfe0

Signed-off-by: Zahid Wakeel <zahid.wakeel@synthara.ai>

ZahidWakeel-synthara force-pushed the zahidw/func_loc_order branch from dee68cf to 180dfe0 Compare March 5, 2026 19:48

Add llvm.func filecheck tests

3994a23

Signed-off-by: Zahid Wakeel <zahid.wakeel@synthara.ai>

		@@ -1,2 +1,2 @@
		// RUN: xdsl-opt --print-op-generic %s \| mlir-opt --mlir-print-op-generic --allow-unregistered-dialect \| xdsl-opt \| filecheck %s
		// RUN: xdsl-opt %s \| mlir-opt --mlir-print-op-generic --allow-unregistered-dialect \| xdsl-opt \| filecheck %s


		func.func private @f(%arg0: i32 loc(unknown) {test.arg_name = "x"})

		// CHECK: Expected function argument attributes before location.

Conversation

ZahidWakeel-synthara commented Mar 2, 2026

Uh oh!

ZahidWakeel-synthara commented Mar 2, 2026

Uh oh!

codecov bot commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JosseVanDelm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Mar 2, 2026 •

edited

Loading