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

Merged
merged 24 commits into from
Feb 13, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 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
7650810
minor change to error printing format
emmau678 Feb 11, 2025
9cf7ddb
Update xdsl/parser/core.py
emmau678 Feb 11, 2025
efd244e
Update tests/filecheck/parser-printer/graph_region.mlir
emmau678 Feb 11, 2025
dc78d82
updates from review comments - condense error handling
emmau678 Feb 11, 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
10 changes: 10 additions & 0 deletions 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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think they are similar except the first one only reports one value, so it's coming from a different if branch in core.py


// -----

// A forward value used with a wrong index

builtin.module {
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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry had not seen this last comment

emmau678 marked this conversation as resolved.
Show resolved Hide resolved
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
Loading