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

Ensure AOT passes all intermediary storages to function calls #9064

Merged
merged 2 commits into from Sep 22, 2021

Conversation

Mousius
Copy link
Member

@Mousius Mousius commented Sep 21, 2021

This iterates over the return storage IDs rather than just using the
first one to ensure all of them get passed to subsequent calls.

Fixes #9036

@PhilippvK
Copy link
Contributor

The actual fix good to me! Thank you.

One aspect I would like to mention before this is getting merged:

This currently replaces the original test_byoc_microtvm test case by the one I come up with to reproduce the issue. I would suggest to add a second test case instead which uses multiple BYOC subgraphs. For this it would be helpful to replace the existing CcompilerAnnotator class with a generic one which is able to annotate any given model. Ideally the MergeCompilerRegions transformation should be enabled/diabled via pytest like this: @pytest.mark.parametrize("merge_compiler_regions", [False, True])

This iterates over the return storage IDs rather than just using the
first one to ensure all of them get passed to subsequent calls.

Fixes apache#9036
@Mousius
Copy link
Member Author

Mousius commented Sep 21, 2021

@PhilippvK, argh! The test said it was a simple BYOC case, I didn't realise it was exercising multiple sub graphs! I've re-added it for now, I think there's probably better test cases but fixing the bug takes priority - are you ok with just re-adding it?

@PhilippvK
Copy link
Contributor

@Mousius oops I just realized, that I am in fact only generating a single subgraph, but a multi-output one. I will then just follow up with another test case and an improved CcompilerAnnotator after this is merged.

@areusch
Copy link
Contributor

areusch commented Sep 22, 2021

@PhilippvK @Mousius so i'm not sure i'm seeing where we've added a multiple-return-value test case here. could you clarify for me? sorry to be dense :).

@areusch areusch self-assigned this Sep 22, 2021
@PhilippvK
Copy link
Contributor

@areusch The test case I came up with to reproduce to bug is quite messy because it does use manually written compiler_begin/compiler_end annotations because the existing CcompilerAnnotator can only create one specific graph. I would like to first make this class more generic to annotate any given model and then reimplement my test case for BYOC and multiple return values. But as this is out of scope of this "bugfix" I should follow up with the actual test case after this is merged.

@areusch
Copy link
Contributor

areusch commented Sep 22, 2021

@PhilippvK i would like to make sure we are exercising the code change here in test, though. it's not clear to me we are...could you clarify?

@Mousius
Copy link
Member Author

Mousius commented Sep 22, 2021

@areusch I ensured the test failed before I made it pass 😸

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

ok got it now, makes sense!

@areusch areusch merged commit dd66bad into apache:main Sep 22, 2021
@Mousius Mousius deleted the aot-weirdism branch September 23, 2021 13:08
ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
…#9064)

* Ensure AOT passes all intermediary storages to function calls

This iterates over the return storage IDs rather than just using the
first one to ensure all of them get passed to subsequent calls.

Fixes apache#9036

* Re-introduce multi sub graph AOT test
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
…#9064)

* Ensure AOT passes all intermediary storages to function calls

This iterates over the return storage IDs rather than just using the
first one to ensure all of them get passed to subsequent calls.

Fixes apache#9036

* Re-introduce multi sub graph AOT test
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.

[Bug] [BYOC] AoT Codegen produces invalid packed function call for relay models using multi-output subgraphs
3 participants