Omr: Power system linkage cannot handle stack arguments

[fjeremic]

As seen in the following test case failure [1]:

13:44:30  26: [----------] 1 test from SystemLinkageTest
13:44:30  26: [ RUN      ] SystemLinkageTest.FooTest
13:44:30  26: /home/jenkins/workspace/PullRequest-linux_ppc-64_le_gcc/fvtest/jitbuildertest/SystemLinkageTest.cpp:76: Failure
13:44:30  26: Value of: foo(200, 3.14159, -10, 6.67300 * pow(10, -11), -123, 0.9876, 999, 1.4142, 200, 3.14159, -10, 6.67300 * pow(10, -11), -123, 0.9876, 999, 1.4142)
13:44:30  26:   Actual: false
13:44:30  26: Expected: true
13:44:30  26: [  FAILED  ] SystemLinkageTest.FooTest (5 ms)

The SystemLinkageTest was modified in #4733 to add additional arguments to ensure some arguments get passed on the stack from the caller. This change broke the AIX and Power Linux LE builds as they suddenly start returning the wrong result. This means the system linkage cannot currently handle stack arguments properly.

On Z we had the same issue and it is fixed in #4733 via 3fe0329. This incidentally also fixes the issues outlined in #3525. I suspect we have a similar issue on Power and we can fix two birds with one stone here.

[1] https://ci.eclipse.org/omr/job/PullRequest-linux_ppc-64_le_gcc/2124/console

[fjeremic]

@aviansie-ben @gita-omr @zl-wang FYI.

[gita-omr]

Thanks @fjeremic. We'll take a look.

[aviansie-ben]

Took a quick peek at this and I've tracked this issue down to the following code: https://github.com/eclipse/omr/blob/9d16f0da5551aa8c5c344aa3820fbe19b5718507/compiler/p/codegen/PPCSystemLinkage.cpp#L452-L459

It seems that for some reason we're trying to load the parameters passed via the stack from the wrong location because of this hack. Removing this and always setting saveParmsInLocalArea = false seems to fix the issue. I'm not sure why this hack was added and whether it's safe to remove. @zl-wang do you know any background surrounding this?

Unfortunately, for Power this looks to be unrelated to #3525, so fixing this problem likely won't have any effect there. We also don't have a SmallIntParmsAlignedRight flag for Power linkages at the moment, so fixing that would require implementing similar functionality ourselves.

[zl-wang]

It is unclear why AIX failed ...

I can understand from the comments why LE Linux failed: it tried to fix one scenario and failed for other scenarios. The background is, in LE ABI, the parameter save area in caller's frame is optional. It can be of size 0 under certain conditions, but it is not always. The comment indicated the existing implementation treated it as always of size 0. The parameter save area in caller's frame must be allocated (for the size to contain the whole parameter list) under these conditions:

1. Not all parameters can be passed in registers (i.e. exceeding number and type);
2. The callee is declared with ellipsis prototyping (i.e. variadic function);
3. The callee is declared without prototype;

From compiling the callee point of view, loading a parameter should be treated similarly to AIX or BE Linux (i.e. it means exceeding the parameter register capacity). Storing a parameter, on the other hand, is different: the caller's save area might not be there. By callee's signature, it should be decided where to store.

[fjeremic]

This is the AIX failure for reference (it fails the same way as Linux LE):
https://ci.eclipse.org/omr/job/PullRequest-aix_ppc-64/1851/consoleFull

[zl-wang]

Also, I believed the right-to-left order is relative to X86's left-to-right (push-pop manner). Not strict meaning.

[aviansie-ben]

Somehow I missed that there was an AIX failure when I originally looked at this. I looked into that one as well and it seems to be a completely different problem that just happens to show up in the same test.

The AIX failure seems to only affect the int32_t parameters. It looks like the caller is storing the entire 64-bit register to the slot in the parameter save area, while the JITted function is only reading the first word of that slot, which is the upper word since this is PPC64BE. In fact, _this_ problem looks to be somewhat related to #3525. The difference here is that this only occurs for int32_t arguments if they're saved by the caller.

[zl-wang]

That AIX failure description made a lot of senses. The caller side storing into the whole slot (wider store with sign or zero extension) is correctly conforming to the ABI. The callee side handling is incorrect. It looks like the parmSymbol being used in different cases of two data types (wider and narrower) without adjusting the address --- guessing from #3525 code snippet. It happens to be correct for LE.

[fjeremic]

The AIX failure seems to only affect the int32_t parameters. It looks like the caller is storing the entire 64-bit register to the slot in the parameter save area, while the JITted function is only reading the first word of that slot, which is the upper word since this is PPC64BE. In fact, _this_ problem looks to be somewhat related to #3525. The difference here is that this only occurs for int32_t arguments if they're saved by the caller.

Same problem on z/OS. This was fixed by spilling the entire 64-bit argument register in the prologue if needed and adjusting the parameter offset to point to the clean bits (+4 in case of int32_t). This makes subsequent loads of the parameter within the callee point to the correct data as it should always be a 32-bit load within the callee.

To fix the AIX case we would need the equivalent of SmallIntParmsAlignedRight. On Z it is only used in a couple of places.

[zl-wang]

This was fixed by spilling the entire 64-bit argument register in the prologue if needed and adjusting the parameter offset to point to the clean bits (+4 in case of int32_t).

Changing the parmSymbol address alone would be enough a fix. Sticking to its true type, loading/storing/spilling would be naturally correct.

[fjeremic]

Changing the parmSymbol address alone would be enough a fix. Sticking to its true type, loading/storing/spilling would be naturally correct.

You're right. The code which generates the argument spills in the prologue on Power [1] uses the offset which it gets from paramCursor->getParameterOffset(), so it should work correctly. It is equivalent to spilling the entire GPR into a wider stack slot functionally, though on Power we do the minimal width store which should in theory be better.

[1] https://github.com/eclipse/omr/blob/3b9951e95f992aab7905dc5da3baf5b474e183f9/compiler/p/codegen/OMRLinkage.cpp#L220-L223
[2] https://github.com/eclipse/omr/blob/3b9951e95f992aab7905dc5da3baf5b474e183f9/compiler/p/codegen/OMRLinkage.cpp#L165