Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: Fix check for forward refs by checking for ssa refs instead of block refs #3851

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
+13 −7
Open

core: Fix check for forward refs by checking for ssa refs instead of block refs #3851

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
e44cd87
fix: change forward block refs to forward ssa refs
emmau678 Feb 5, 2025
b86c47e
dialects: (builtin) remove AnyMemRefTypeConstr (#3832)
alexarice Feb 4, 2025
0bc2616
core: Disallow duplicate keys in attribute dictionaries (#3830)
compor Feb 4, 2025
324792f
pip prod(deps): bump marimo from 0.10.19 to 0.11.0 (#3834)
dependabot[bot] Feb 5, 2025
83ee4c0
dialects: (builtin/memref) rename Memref to MemRef everywhere (#3833)
alexarice Feb 5, 2025
60fd998
dialects: (linalg) add hidden region to transpose op (#3838)
jorendumoulin Feb 5, 2025
9eceb8c
dialects: (linalg) let PoolingOpsBase inherit from NamedOpBase (#3839)
jorendumoulin Feb 5, 2025
a216535
dialects: (linalg) add hidden region to BroadcastOp (#3840)
jorendumoulin Feb 5, 2025
41b16cd
dialects: (linalg) let ConvOpsBase inherit from NamedOpsBase (#3841)
jorendumoulin Feb 5, 2025
75fbbe0
dialects: (linalg) enable generic printing in mlir conversion fileche…
jorendumoulin Feb 5, 2025
617303c
dialects: (builtin) remove AnyIntegerAttrConstr (#3842)
alexarice Feb 5, 2025
51db69f
dialects: (builtin) remove AnyIntegerAttr (#3843)
alexarice Feb 5, 2025
4504a60
dialects: (builtin) remove AnyFloatAttr(Constr)? (#3844)
alexarice Feb 5, 2025
6e51e9b
installation: fix packages discovery (#3847)
alexarice Feb 5, 2025
9f50033
dialects: (builtin) print hex str for DenseIntOrFPElementsAttrs with …
jorendumoulin Feb 5, 2025
34ab330
core: Fix multiline error printing (#3849)
emmau678 Feb 6, 2025
4088e1b
update line numbers in filecheck
emmau678 Feb 6, 2025
64b9606
Merge branch 'main' into emma/add_fix_forward_ssa_refs
emmau678 Feb 6, 2025
2291eb9
move check to before the code
emmau678 Feb 6, 2025
b356946
fix last change to move correct check above code
emmau678 Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion tests/filecheck/parser-printer/graph_region.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,16 @@ builtin.module {

// -----

// A graph region that refers to values that are not defined in the module.

builtin.module {
%0 = "test.termop"(%1, %2) : (i32, i32) -> i32
}

// CHECK: values %1, %2 were used but not defined
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the check statement go before the code? I'm also not sure the comment is necessary, the error message describes what is going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved it, thanks

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless github is being weird, it seems you moved a different one?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment can stay since we are doing it for the rest of the file.

I have a different question though; how does this differ from the test case directly above?
(maybe I need more coffee?)


// -----

// A forward value used with a wrong index

builtin.module {
Expand All @@ -73,7 +83,7 @@ builtin.module {
"test.op"() : () -> ()
}

// CHECK: /graph_region.mlir:72:4
// CHECK: /graph_region.mlir:82:4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fun I was hoping to avoid

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think it's worth it :)

// CHECK-NEXT: ^blockA:
// CHECK-NEXT: ^^^^^^^
// CHECK-NEXT: re-declaration of block 'blockA'
Expand Down
2 changes: 1 addition & 1 deletion xdsl/parser/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ def parse_module(self, allow_implicit_module: bool = True) -> ModuleOp:
value_names = ", ".join(
"%" + name for name in self.forward_ssa_references.keys()
)
if len(self.forward_block_references.keys()) > 1:
if len(self.forward_ssa_references.keys()) > 1:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is purely for getting the plural correct it seems

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok thanks @alexarice, I updated the description

Copy link
Collaborator

@compor compor Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just to purely have a plural and singular form in the error?

I don't remember the initial discussion, but wouldn't something like:

value(s) used but not defined: %1, %2

value(s) used but not defined: %1

work for both cases?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this what we do in most places. I was surprised that it tries to get the plural correct here (yet gets it wrong anyway)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH yeah it seems like a reasonable simplifying change. Maybe even something like this:

values used but not defined: [%1, %2]
values used but not defined: [%1]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

easier to grep

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emmau678 let's update to do something like this? maybe values [%1, %2] used but not defined?

self.raise_error(f"values {value_names} were used but not defined")
else:
self.raise_error(f"value {value_names} was used but not defined")
Expand Down