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

Add declare/define versions of agent/host function shims #1049

Merged
merged 1 commit into from
Mar 20, 2023

Conversation

Robadob
Copy link
Member

@Robadob Robadob commented Mar 17, 2023

This makes it easier to split models across multiple compilation units.

Useful to Fujitsu model, I manually did host function declarations in Primage too but never required agent function declarations until now.

This is a relatively advanced feature (and we don't have any documentation on splitting large models), so I feel it requires documentation.

I build and ran the test suite for sanity reasons, so it should be fine.

@Robadob Robadob requested a review from ptheywood March 17, 2023 18:04
@Robadob Robadob self-assigned this Mar 17, 2023
Copy link
Member

@ptheywood ptheywood left a comment

Choose a reason for hiding this comment

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

Changes look fine, current test suite all passes under linux.

I've not manually tried using them separately across compilation units however, but don't see why it wouldn't work.

I aggree about it worth being documenting.

@Robadob
Copy link
Member Author

Robadob commented Mar 17, 2023

I aggree about it worth being documenting.

I typo'd, OP was supposed to say don't feel worth documenting.

@Robadob
Copy link
Member Author

Robadob commented Mar 19, 2023

Have now tested it, doesn't device link unless __forceinline__ is removed from the agent function/agent function condition macros.

So two choices:

  • Check the performance implications of removing __forceinline__ from agent function implementations.
  • Use redundancy, and only remove it from the DECL/DEF versions, which should be used sparingly.

@Robadob Robadob marked this pull request as draft March 19, 2023 20:03
@ptheywood
Copy link
Member

Makes sense that __forceinline__ doesn't link if its' effectivley being multiply defined.

If removing it does cost perf, LTO might be a way to reclaim the lost performance, would only know via experimentation.

Duplication seems like the "safer" choice / makes it opt-in to the (potentially slower) non inlined version(s)

@Robadob Robadob marked this pull request as ready for review March 20, 2023 13:06
This makes it easier to split models across multiple compilation units.
Copy link
Member

@ptheywood ptheywood left a comment

Choose a reason for hiding this comment

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

Looks fine and test suite is still happy, though the new definitions are not present in the test suite so haven't actually tested them myself.

Could probably do with being put into a "we should add tests for these at some point" issue.

@Robadob
Copy link
Member Author

Robadob commented Mar 20, 2023

Could probably do with being put into a "we should add tests for these at some point" issue.

Will do, I've tested them locally with fujitsu.

@Robadob Robadob mentioned this pull request Mar 20, 2023
@Robadob Robadob merged commit c5007d2 into master Mar 20, 2023
@Robadob Robadob deleted the fn_shim_decl_def branch March 20, 2023 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants