dialects: Update support for returning multiple results using stencil.return

[meshtag]

This PR intends to update the support for multiple stencil results. The earlier version produced some bugs when lowered via Psyclone, this patch should not face a similar problem. Here's a link to @mesham's original issue related to this.

edit @georgebisbas : Also check: #659 (comment)

[PapyChacal]

LGTM, does it solve @mesham's issue?

[codecov]

Codecov Report

Patch coverage: 50.00% and project coverage change: +0.89 🎉

Comparison is base (7a3c65b) 87.64% compared to head (b18a45f) 88.53%.

❗ Current head b18a45f differs from pull request most recent head 47a2f7a. Consider uploading reports for the commit 47a2f7a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #659      +/-   ##
==========================================
+ Coverage   87.64%   88.53%   +0.89%     
==========================================
  Files         106      103       -3     
  Lines       15446    14657     -789     
  Branches     2288     2182     -106     
==========================================
- Hits        13538    12977     -561     
+ Misses       1482     1270     -212     
+ Partials      426      410      -16     

Impacted Files	Coverage Δ
xdsl/dialects/experimental/stencil.py	79.05% <50.00%> (-0.30%)	⬇️

... and 46 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[tobiasgrosser]

Can we add test cases her? To actually demonstrate that @mesham's testcase works?

[meshtag]

Can we add test cases her? To actually demonstrate that @mesham's testcase works?

Sure, I can do this. I just want to know his earlier test case which broke this in the first place.

[tobiasgrosser]

Can we add test cases her? To actually demonstrate that @mesham's testcase works?

Sure, I can do this. I just want to know his earlier test case which broke this in the first place.

He might be busy. Maybe you can derive it from his comments (if he does not reply) without too much trouble?

[meshtag]

He might be busy. Maybe you can derive it from his comments (if he does not reply) without too much trouble?

I am not able to easily reproduce it (atleast from the Python land).

[georgebisbas]

I would keep on hold until we have a test for this

[PapyChacal]

Some minor comments, but looks sensible! Thanks!

[PapyChacal]

We might just want to add a test for stencil.ReturnOp.get now!
You can have a look at similar tests in tests/dialects if you feel like adding it, but I'm happy to add it tomorrow otherwise

[georgebisbas]

This should ideally fix the problem related to stencil.return op build.
Could you point to this problem?

[meshtag]

Could you point to this problem?

There was some issue faced by @mesham. @mesham, can you please check if the problem still persists. Please state how to reproduce the issue as well (in case it is unsolved).

[PapyChacal]

If I'm not mistaken, @mesham is having time off now, so I'll answer my understanding on that:

@mesham had example code where having multiple returns would make sense, and tried to generate and lower them, as the API suggested it was possible. This was not implemented yet though (especially the lowering part) and the get was erroring, which led to think that get was the issue.

Meanwhile, he generated his code by splitting all stencils with multiple outputs in seperate stencils and got it working, so this PR shouldn't have any regression on that side. I just tested on the latest IR sample I got from Psyclone and it lowered just fine :)

[PapyChacal]

@georgebisbas here's a link to @mesham's original issue : https://xdsl.zulipchat.com/#narrow/stream/362011-xDSL/topic/stencil.20dialect/near/346783038. Your request for changes is blocking the merge, is it on purpose?

[georgebisbas]

Thanks, ideally in these, add some description to this PR, so that someone understands what happens.
In the most ideal world, we would need an MFE of NIck's issue.
But at least two lines of description should be enough.

[meshtag]

This was not implemented yet though (especially the lowering part) and the get was erroring, which led to think that get was the issue.

Can you please specify what is referred to as "lowering part" here?

Thanks, ideally in these, add some description to this PR, so that someone understands what happens.

Sure! I was not sure about the bug details and how to reproduce it at the time of this PR's creation (am still not very sure tbh, will trust @PapyChacal ). It was sort of an ad-hoc solution to see if we could solve the problem. Nevertheless, the original patch's merge was reverted and I am now changing this PR's purpose to "adding the multiple return support again". I updated the description to mention this. Let me know if I should change anything else.

[PapyChacal]

By "the lowering part", I mean lowering a stencil.return with multiple operands, to be able to lower a stencil.apply outputting multiple stencil.temp

[PapyChacal]

But, to sum it up, this PR:

• Generalize the stencil.return lowering to handle multiple in a backward-compatible manner (all single-operands returns are lowered just as before)
• Change the stencil.ReturnOp operand to be variadic(as it is intended to), so this might be the slightly breaking change, as it now should be passed a length 1 list for a single return. It also introduces a test, where it was untested before.

Writing this I realize the new constructor could be made to work with single values too, so the PR is completely backward compatible.

[georgebisbas]

But, to sum it up, this PR:

• Generalize the stencil.return lowering to handle multiple in a backward-compatible manner (all single-operands returns are lowered just as before)
• Change the stencil.ReturnOp operand to be variadic(as it is intended to), so this might be the slightly breaking change, as it now should be passed a length 1 list for a single return. It also introduces a test, where it was untested before.

Writing this I realize the new constructor could be made to work with single values too, so the PR is completely backward compatible.

ok, added a link to this comment in the first post

[meshtag]

Can we merge this?

[PapyChacal]

Can we merge this?

Yes, I'm on it 🙂

[meshtag]

I'm curious, is it a necessary/advisable requirement to merge main before letting this (or any other PR for that matter) in? I was under the impression that we are fine unless we observe merge conflicts, git should automatically take care of merging things properly unless there is a merge conflict?