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

Discussion on the JIT helpers #76

Closed
r30shah opened this issue Sep 19, 2017 · 10 comments
Closed

Discussion on the JIT helpers #76

r30shah opened this issue Sep 19, 2017 · 10 comments

Comments

@r30shah
Copy link
Contributor

r30shah commented Sep 19, 2017

Recently I was making changes in to CHelper linkage to call helperCFloatRemainderFloat from JIT (#106) to get rid of unneeded assembly glue from Math.m4 to call helper.
While making changes, I realized that the linkage is really specific to the JIT helpers in cnathelp.cpp and needed extra patches to make linkage compatible to call any arbitrary C functions.

  • One issue is order of arguments, A call node will have first argument to the function in first child and subsequently other arguments in subsequent child while JIT helpers have this arguments in reverse order (First child is last argument, last child is second last argument with vmThread is first argument). This makes linkage incompatible for using it to call arbitrary C functions and while using it to call other C functions, we need to pass flag to evaluate child in correct order of prepare list of arguments and pass it.

I am opening this issue to start discussion on issues or concerns for refactoring of JIT helpers in cnathelp.cpp and znathelp.m4 to use more consistent C function linkage.

@r30shah
Copy link
Contributor Author

r30shah commented Sep 19, 2017

@fjeremic @0dvictor @Graham-Chapman Please share your thought regarding to this.
FYI @0xdaryl

@fjeremic
Copy link
Contributor

fjeremic commented Sep 19, 2017

@0dvictor does the x86 codegen share the CHelper linkage with the vanilla C linkage? Do you have the ability to call arbitrary C functions from the x86 codegen? I don't quite understand why CHelper functions are at all different than any other normal C functions. They don't seem to be really, it's just that the parameters are in reverse order. This is likely because the old TR_Helper linkage sent arguments in reverse order.

See usage of buildArgs in:
https://github.com/eclipse/omr/blob/562030e19253171ad05a6fd2ec5cbf8e82a6bc1f/compiler/z/codegen/OMRLinkage.cpp#L2383

The isParmsInReverseOrder is only set here:
https://github.com/eclipse/openj9/blob/dab5701f056efe5ad821b7c61c391ba0300593b8/runtime/tr.source/trj9/z/codegen/J9S390PrivateLinkage.hpp#L151

@gacholio is there a non-legacy reason why this is?

@dvictor
Copy link

dvictor commented Sep 19, 2017 via email

@r30shah
Copy link
Contributor Author

r30shah commented Sep 19, 2017

Apologies for that, mixed up with username, Attn @0dvictor

@0dvictor
Copy link
Contributor

The current X86 CHelper linkage does not handle vanilla C linkage; although the only different is the arguments order.
A pending Pull Request is to add to full support vanilla C linkage by taking a flag to note arguments orders. The PR will be created once tests cleared.

@0dvictor
Copy link
Contributor

I believe TR_CHelper was designed to be as close as TR_Helper as possible, so that the arguments order is same as TR_Helper, which is the opposite of C linkage. I personally think it is the right time to polish TR_CHelper further now, making it the true C linkage.

@ymanton
Copy link
Member

ymanton commented Sep 19, 2017

If you want to make a call to a plain C function why don't you use the system linkage from OMR instead of making CHelper linkage do the same thing?

@0dvictor
Copy link
Contributor

On X86, System linkage switches stack when calling C function, while CHelper linkage calls C function directly on Java Stack. Such stack switch contributes to quite a portion of performance degradation; and hence we prefer to use CHelper linkage for functions that can be called on Java stack.

@gacholio
Copy link
Contributor

Anything in the JIT private linkage or helper linkage was decided years ago - there's no reason I know of why it should be differently ordered.

@DanHeidinga
Copy link
Member

Closing as related PR has been merged

fjeremic pushed a commit to fjeremic/openj9 that referenced this issue Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants