Add thunk/adapter for FFI Upcall on Z - #15596
Conversation
|
Marked as a draft because I need to confirm that a failure in the builds is not related to my changes, and some formatting changes need to be made. |
|
FYI: @tajila, @pshipton, @DanHeidinga |
There was a problem hiding this comment.
General comment: make the indentation uniform. Maybe your editor setting.
Specific comment to this code: avoid duplicate code in if and else.
There was a problem hiding this comment.
Both of these should be fixed now.
There was a problem hiding this comment.
this looks wrong: since single-precision FP is represented in the left part of the register, you cannot jump over structOffset to return the address later on.
There was a problem hiding this comment.
The structOffset will be zero when the struct size is 1, 2, 4 or 8 bytes.
On zLinux only structs of size 1, 2, 4, and 8 can be passed by value in a register. This is usually done in a GPR.
The only scenario where this is done in an FPR is if we have a pure float/double struct with a single element (i.e. structs of size 4 and 8 bytes).
So I don't think we will end up in a problematic situation. Unless I'm missing sometime else here?
There was a problem hiding this comment.
No. the point is you cannot handle struct-of-1/2/4 bytes the same way with FPR and GPR arguments. the reason is: FPR is only populated in the left and GPR is only populated on the right. Imagine a single pure float struct to be passed ... it is passed by value in FPR with left half register populated ... you store it back with STEY at offset 0 in the stack slot ... later on, you have a structOffset value of 4 ... the return address actually points at the right half of the slot.
There was a problem hiding this comment.
This has been fixed now. structOffset will be zero for FPR argument types (including structs passed by value in FPR).
There was a problem hiding this comment.
mentioned above: you cannot handle FPs similarly as to integer arguments.
There was a problem hiding this comment.
for ALL_SP with a single float, this calculation is obviously having structOffset set to 4.
There was a problem hiding this comment.
Ah I see what you mean. It does look like I missed that corner case. I'll rework this when I push new changes with the formatting fixed.
I'll also be adding to the upcall tests to capture all the zLinux corner cases (either in this PR or I'll pass them on to Cheng) so hopefully we won't have any more of these.
There was a problem hiding this comment.
No. the point is you cannot handle struct-of-1/2/4 bytes the same way with FPR and GPR arguments. the reason is: FPR is only populated in the left and GPR is only populated on the right. Imagine a single pure float struct to be passed ... it is passed by value in FPR with left half register populated ... you store it back with STEY at offset 0 in the stack slot ... later on, you have a structOffset value of 4 ... the return address actually points at the right half of the slot.
There was a problem hiding this comment.
for ALL_SP with a single float, this calculation is obviously having structOffset set to 4.
404b686 to
c3e744e
Compare
|
This commit should pass all up call tests found in @ChengJin01's branch: https://github.com/ChengJin01/openj9/find/jep389_419_ffi_upcall_v4_struct_v2_outofline_mh_v9_fin_v8 Note however that the Memory Padding in |
|
I also did my own unit testing as I was building the solution to make sure I'm covering all cases. On zLinux the parameter can be stored in one of two call frames depending on the situation. So there are some unique tests with 10+ arguments to test these scenarios. I've put those tests here. The readme describes the test cases. @ChengJin01 if you think any of these tests would be worth adding to your existing tests then let me know and I can provide you with the necessary files. |
r30shah
left a comment
r30shah
left a comment
There was a problem hiding this comment.
@dchopra001 Flushing out the first round of review. I have so far looked at the code of functions to generate the instructions in detail, most of the things looks OK, some things I observe while using MVC instructions that I think needs to be checked out.
There was a problem hiding this comment.
How about using #define macro for some of the constants that represents the value of register for instructions, to improve readability and avoid using local variable for them? It would be useful for other functions in the code as well.
There was a problem hiding this comment.
I've added these here: 39be597c2b57137681a8c4d40d31df5fc384b62e
There was a problem hiding this comment.
This increment represents the size of LG instruction right? In that case, if call to LG function returns the instruction size, shouldn't it use that instead of this hardcoded 6?
There was a problem hiding this comment.
Should be fixed now as I mentioned on the comment on line 172
There was a problem hiding this comment.
Similar to previous comment. As the caller of this function consumes the instruction size returned by it to update instruction pointer, for this particular function passing it instructionPtr+totalInstructionSize may also work and you can avoid incrementing the instruction by hardcoded constants.
Also is it possible to simplify argument so that it is clear, what is source memory reference, what is destination memory reference, and how many bytes we are copying?
There was a problem hiding this comment.
The naming here should be improved now to make the function call more readable. I've also improved the variable names in the helper routines and updated how instructionPtr as per your suggestion above.
There was a problem hiding this comment.
Do we need this commented part?
There was a problem hiding this comment.
I've added these as it makes diagnosing problems easier. Right now I've commented them out for the PR but I'm enabling these while testing locally. I would vote to keep some diagnostic prints in some fashion (maybe we can enable them conditionally based on environment variables). If you think we don't need them then I'll remove them just before we merge.
There was a problem hiding this comment.
Looking the code down below that is used to emit MVC instruction, I believe you are using wrong value here as maxBytes. MVC can handle 256 (1 byte used for L field, so 0-255 bytes) bytes at a time. So your calculation for loop and residue also needed to be changed.
There was a problem hiding this comment.
So in revisiting this area I have a couple of observations:
-
I don't think the issue mentioned above created any functional issues because the displacement integer calculation accounted for this discrepancy. This probably explains why I didn't see any failures in my test case because I do have edge cases for this scenario. Anyway, you're right that we should set maxBytes to 256. I'll have this fixed in the next commit.
-
There is however a bug if we try to copy a structure larger than 4096 bytes. This is because the displacement integer in the instruction is only 12 bits in length. So in scenarios where we have to copy more than 4096 bytes of data, I propose the following solution:
- Copy the structure using consecutive MVC instruction for the first 4096 bytes
- If there's more than 4096 bytes, then copy the data over using an MVCLE instruction (edited)
There was a problem hiding this comment.
Good catch about the 4096 bytes,
Your proposed solution means we will use 16 MVCs and for the remaining bytes use MCVL/MVCLE . Means we will use 100 bytes of instructions with that. Apart from that, you have to emit instruction to increase both source and destination by 4096 and also load the remaining bytes in the register for MVCLE.
Another caveat with using MVCLE looking at the PoPs, you would have to also insert branch instruction to account for interruption in copying from source to destination to account for operation completion because of CPU determined number of bytes have been moved without reaching the end of the first operand in which case condition code would be set.
We should gauge if this is beneficial or simple loop of MVC (MVC, update src, update dst, check residue is less than 256 and branch back).
There was a problem hiding this comment.
Also I need to confirm this from the instruction documents but the comment in [1] suggests a very high threshold should be used for deciding MVCL.
There was a problem hiding this comment.
This routine is now split into two here. One copies small structs using upto 16 MVC instructions, and the other one copies larger structs in a loop with 16 MVC instructions at a time. I believe I'm missing one more corner case here. I'll update this comment once I've addressed that.
There was a problem hiding this comment.
Comment needs some clarification on why 23 or why size/256 + 2
There was a problem hiding this comment.
Added comments around how this is calculated and updated this area as well: 193521ffdae93a7ae820818ab09473e47b0b2a9b
There was a problem hiding this comment.
If we are not using roundedCodeSize later on this code, we can get rid of using a new variable and combine two lines here to set thunkSize.
There was a problem hiding this comment.
This should be fixed now.
There was a problem hiding this comment.
Should we use LAY to update the C Stack pointer instead of AGFI?
There was a problem hiding this comment.
LAY instructions being used here: 68ef0623c789bfab3d1dcff2dc46fee881686cbf
There was a problem hiding this comment.
I think this constant initialization for stack frame needs a comment. Perhaps a general comment laying out stack frame would increase readability of the code.
There was a problem hiding this comment.
I added some comments around magic constants here: 6e50a19f91bf9b540b7fd894d891a7b8a9f08aa3
There was a problem hiding this comment.
We do not return after this point right? In this case, we should use BCR.
There was a problem hiding this comment.
This should be fixed in: c54d5bd7f1575e9af8aa7f5c2fd37c396ce8be05
There was a problem hiding this comment.
A comment or description on how the MVC loop works and residue is handled should be added here. Also is it possible to refactor the small struct copy and large struct copy and let single function handle both scenarios?
There was a problem hiding this comment.
Comments Should be added in aed112117b9b089ce249283522afb353c7ac1c6e.
I would propose to keep the routines separated. There isn't as much duplicate code between the two and combining them together will require a bunch of if guards that will add additional complexity.
There was a problem hiding this comment.
This means the function is fixed to use R2 as source register and R4 as destination register deeming it to be used in specific locations only. Should this be parameterized to allow it to be used if we ever have to copy with different source and destination register?
There was a problem hiding this comment.
Now the source and destination register operands are parametrized for the struct copy routines: 109cbe6452e154afe9686653d3693605391b8423
There was a problem hiding this comment.
This should be fixed now.
|
jenkins test sanity zlinux jdk8,jdk19 |
|
@joransiu any concerns/review with this change? Since @dchopra001 have tested the changes with different test case, and the sanity test passes, should we attempt merging this one? |
I would like to see if @dchopra001 will get a chance to address the few minor outstanding comments, and squash the commits. @zl-wang / @ChengJin01 Do we have any specific tests we should trigger to exercises these paths? |
|
@joransiu, @dchopra001 should be able to verify this PR locally with his own tests specific to Z at https://github.com/dchopra001/ffiUpCallTests (he already verified with my test suites locally with my branch). In terms of generic tests suites for upcall, our test cases at #15310 have not yet merged to the repo as we have not yet finished the code review for all PRs listed at #15068. So there is no way to directly verify with these test suites for the moment due to the incomplete code in the repo. |
|
@dchopra001 & @joransiu, could this PR be merged if there is no more concern to be addressed? |
r30shah
left a comment
r30shah
left a comment
There was a problem hiding this comment.
Minor mixed up in the license header text, apart from that, I am OK with most of the changes made by @dchopra001. @ChengJin01 What is the timeline within which we need to get this change in?
@r30shah, we should have sufficient time for this PR but I'd like to have it merged as soon as possible if we could. So I will need to double-check with jtreg tests to see whether there is any platform specific issue to be addressed on zLinux. |
|
I did test the last time I pushed code changes to this branch a week or two ago. Just to be sure I'm doing a clean build and will test again. I'll update here once the build is done and the tests are clear. |
joransiu
left a comment
joransiu
left a comment
There was a problem hiding this comment.
LGTM. One minor comment nit.
There was a problem hiding this comment.
The loop concludes where... may be better worded as The loop concludes when...
There was a problem hiding this comment.
This should now be fixed in the final squashed commit.
|
Not sure why the Copyright check and Line Ending checks suddenly flagged issues in other files, and shows conflicts on merge. @dchopra001 , can you squash and rebase? |
|
@AdamBrousseau, is there any update in terms of the Copyright & Line Endings Check ? It seems the failing lines listed in there totally have nothing to do with this PR. |
|
@ChengJin01 the checks don't work properly when there are merge conflicts in the PR. Once they are resolved, the check should return proper results. |
|
@dchopra001, please double-check whether there is any conflict to be resolved in this PR. |
|
Waiting for @dchopra001 's comment for verifying testing. Overall, changes looks good to me. |
c555c03 to
6fd85fe
Compare
Signed-off-by: Dhruv Chopra <Dhruv.C.Chopra@ibm.com>
6fd85fe to
56b7e87
Compare
|
Merge conflicts resolved and all my local tests have passed as well. |
|
jenkins test sanity zlinux jdk8,jdk19 |
6 participants
Signed-off-by: Dhruv Chopra Dhruv.C.Chopra@ibm.com