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.

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

Roundtrip the undef node #660

Merged
merged 28 commits into from
Nov 18, 2024
Merged

Roundtrip the undef node #660

merged 28 commits into from
Nov 18, 2024

Conversation

halvorlinder
Copy link
Contributor

@halvorlinder halvorlinder commented Oct 16, 2024

This PR roundtrips the undef node and provides a unit test for it.
In addition, it updates the commit hash used for building the mlir-rvsdg dialect to include the fix of the undef operation output type.

phate
phate previously approved these changes Oct 16, 2024
Copy link
Owner

@phate phate left a comment

Choose a reason for hiding this comment

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

Only minor comments

jlm/mlir/backend/JlmToMlirConverter.cpp Outdated Show resolved Hide resolved
jlm/mlir/frontend/MlirToJlmConverter.cpp Outdated Show resolved Hide resolved
jlm/mlir/frontend/MlirToJlmConverter.cpp Show resolved Hide resolved
@sjalander
Copy link
Collaborator

@phate @haved This test uses both the backend and frontend.
I suggest that we place it in test/jlm/mlir instead of either the backend or frontend.
The name of the file could be MlirRoundtripTest.cpp or JlmToMlirToJlmTest.cpp.

@phate
Copy link
Owner

phate commented Oct 16, 2024

@sjalander Alternatively, we could split the test in two: one for frontend and one for backend?

@sjalander
Copy link
Collaborator

@phate In this case we could, but that adds extra work as one then need to manually construct both the RVSDG and MLIR graph for the two individual tests. That's what we have done so far, but it is tedious work that doesn't contribute to improving the test coverage.

This wouldn't work in general for the roundtrip tests we have talked about, where one create an RVSDG graph that gets converted to MLIR and then back to RVSDG-new and a simple comparison between RVSDG and RVSD-new is made.
Such tests have the added benefit that they ensure that the backend and frontend generates consistent results.

@phate
Copy link
Owner

phate commented Oct 18, 2024

Place them in test/jlm/mlir then.

@haved haved merged commit cf028fe into phate:master Nov 18, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants