Page MenuHomePhabricator

[Powerpc] store byval parameter to parameter save area when needed
ClosedPublic

Authored by shchenz on Oct 9 2021, 2:34 AM.

Details

Summary

For now, for 64 bit SVR4 ABI, when lowering a CALL, a byval parameter bigger than 8 bytes will be copied to the caller's stack frame. This will cause some issues because when the byval parameter is smaller than 64 bytes (assume there is only one parameter). HasParameterArea is not set to true, so on the caller side, the stack local area overlaps with the parameter save area.

In D105271, I tried to set the HasParameterArea to true even the byval parameter can be entirely passed in registers. That patch can fix the above overlap issue.

But with the fix D105271, we met a new issue when mix-compiling with other compilers like GCC:
On the callee side,
If the callee is compiled with clang and the parameter is set to be with byval parameter in some IR generator, (assume there is only one parameter for the callee and the parameter size is bigger than 8 bytes but smaller than 64 bytes), then the callee assumes the caller will allocate parameter save area, so the callee will store the parameter passed by register to caller's parameter save area.

On the caller side,
If the caller is also compiled with clang, then we are OK as it will use the new logic I added in D105271. Caller and callee have a consistent parameter save area allocation policy.
But if the caller is compiled with other compilers, like GCC, then we will meet some issues. Because GCC will not allocate parameter save area for structures smaller than 64 bytes (assume there is only one parameter).

So I reverted D105271 and propose this new fix.

Now we won't copy the byval parameter (bigger than 8 bytes) to caller's parameter save area. Instead, we will only copy the byval parameter when it can not be passed entirely in registers which means we have to use parameter save area according to the 64 bit SVR4 ABI.

Diff Detail

Event Timeline

shchenz created this revision.Oct 9 2021, 2:34 AM
shchenz requested review of this revision.Oct 9 2021, 2:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2021, 2:34 AM

gentle ping

ping...
Could someone help to review this one-line change? Or Can I check in this patch first and see if there are any ABI issues exposed?

gentle ping

gentle ping

I asked for some help from a GCC developer. He told me GCC only copies byval structure bigger than 64 bytes to the parameter save area instead of 8 bytes for ELF64. So now after the change, clang should have the same logic as GCC.

jsji accepted this revision as: jsji.Dec 6 2021, 6:27 PM

LGTM since you have already double checked the compatiblity.

This revision is now accepted and ready to land.Dec 6 2021, 6:27 PM

LGTM since you have already double checked the compatiblity.

Thanks very much. @jsji

The patch passed all our internal tests I can find and I also verified the GCC and commercial XLC's behavior and also confirmed the behavior with the GCC developer.

This revision was landed with ongoing or failed builds.Dec 8 2021, 5:01 PM
This revision was automatically updated to reflect the committed changes.