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

core: (rewriting) Fix recursive type conversion in block arguments for nested regions #2871

Merged
merged 3 commits into from
Jul 10, 2024

Conversation

webmiche
Copy link
Collaborator

@webmiche webmiche commented Jul 9, 2024

closes #2857

Solves the issue mentioned there and adds the reported testcase as a regression test.

Before, this was actually quite broken as not applying the recursive rewrite, but the normal rewrite, to the region would actually mean that the produced func_op did not even verify (the function_type attribute got rewritten properly, whereas the entry_block args would still have the old type).

The reason why the old type was being printed is that the print_func_op_like-utility function defaults to printing the entry_block arguments in some cases.

@webmiche webmiche requested review from superlopuh and PapyChacal July 9, 2024 09:03
@webmiche
Copy link
Collaborator Author

webmiche commented Jul 9, 2024

@francescodaghero feel free to also comment/leave a review :)

Copy link

codecov bot commented Jul 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.86%. Comparing base (f7f1471) to head (c16e08b).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2871      +/-   ##
==========================================
- Coverage   89.87%   89.86%   -0.01%     
==========================================
  Files         396      397       +1     
  Lines       49827    49966     +139     
  Branches     7683     7713      +30     
==========================================
+ Hits        44780    44903     +123     
- Misses       3837     3846       +9     
- Partials     1210     1217       +7     

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

@webmiche webmiche changed the title core: (rewriting) Fix recursive type conversion in regions core: (rewriting) Fix recursive type conversion in block arguments for nested regions Jul 9, 2024
@superlopuh superlopuh added the core xDSL core (ir, textual format, ...) label Jul 9, 2024
@webmiche
Copy link
Collaborator Author

@superlopuh do you think we can merge this without @PapyChacal s approval? It seems like fairly obvious fix to me, but I am not very deep in the intricacies of the TypeConversion stuff.

@superlopuh
Copy link
Member

Hmm, reading the source code more, it seems that _convert_type_rec is really the thing to call everywhere, and I'm not sure why it wasn't in the first place. There's a check for self.recursive in the private method, so it should do the right thing without checking for it twice. I would probably change it to that and then merge, I'm not sure when Emilien is back from holidays but we can fix it then if anything breaks that we're not aware of.

@webmiche
Copy link
Collaborator Author

Hmm, yeah fair enough. I am honestly not sure what we want this API to look like exactly, but your change sounds reasonable.

@webmiche
Copy link
Collaborator Author

I will wait for the CI to pass and then merge.

@webmiche webmiche merged commit 57e3075 into main Jul 10, 2024
10 checks passed
@webmiche webmiche deleted the michel/rec_type_conv_block_args branch July 10, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core xDSL core (ir, textual format, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type Conversion of function arguments
2 participants