-
Notifications
You must be signed in to change notification settings - Fork 20
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
Conversation
There was a problem hiding this 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.
I typo'd, OP was supposed to say don't feel worth documenting. |
Have now tested it, doesn't device link unless So two choices:
|
Makes sense that 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) |
edfa83f
to
3bca099
Compare
This makes it easier to split models across multiple compilation units.
3bca099
to
a66df32
Compare
There was a problem hiding this 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.
Will do, I've tested them locally with fujitsu. |
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.