This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][AIX] Support ByVals with greater alignment then pointer size
Needs ReviewPublic

Authored by ZarkoCA on Jul 8 2021, 2:23 PM.

Details

Reviewers
sfertile
cebowleratibm
Group Reviewers
Restricted Project
Summary

Implementation is NOT compatible with XL. Instead it handles all ByVals
with greater alignment then pointer width the same way XL handles Byvals
that have vector members. For overaligned objects that do not contain
vectors XL does not align them properly if they are passed in the GPR
argument registers.

This patch was originally written by Sean Fertile (@sfertile) and I am
posting on phabricator.

Diff Detail

Event Timeline

ZarkoCA created this revision.Jul 8 2021, 2:23 PM
ZarkoCA requested review of this revision.Jul 8 2021, 2:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2021, 2:23 PM
sfertile accepted this revision as: sfertile.Sep 16 2021, 7:34 AM

LGTM, but since as you mentioned I originally wrote this I would fell better if @cebowleratibm reviewed and approved before committing as well.

This revision is now accepted and ready to land.Sep 16 2021, 7:34 AM
ZarkoCA updated this revision to Diff 373011.Sep 16 2021, 11:21 AM

Fixed test cases that were failing due to changed in expected ouput:

  • store/load 4/8 is now store/load (s32/s64)
  • undef is now undefined-address
cebowleratibm requested changes to this revision.Sep 23 2021, 8:54 AM

As we lift the backend fatal error we expose the risk of generating silently incompatible code with the XLC compiler on AIX. I think a clang warning diagnostic is warranted for 16 byte aligned byval args where the struct is 16 byte aligned and does not contain a vector member. I don't think this case is common so the diagnostic shouldn't be too verbose.

I consider the insufficiently aligned address of the parameter an XLC bug and this change makes clang consistent with GCC on AIX, so I agree with the intention of the change.

If you agree with the clang diagnostic, I think it's better to land that before committing this just so that we don't expose ourselves to emitting silently incompatible code.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6570

nit: emit the value "16" based on the value of StackAlign rather than hardcoding 16 again.

This revision now requires changes to proceed.Sep 23 2021, 8:54 AM

As we lift the backend fatal error we expose the risk of generating silently incompatible code with the XLC compiler on AIX. I think a clang warning diagnostic is warranted for 16 byte aligned byval args where the struct is 16 byte aligned and does not contain a vector member. I don't think this case is common so the diagnostic shouldn't be too verbose.

I consider the insufficiently aligned address of the parameter an XLC bug and this change makes clang consistent with GCC on AIX, so I agree with the intention of the change.

If you agree with the clang diagnostic, I think it's better to land that before committing this just so that we don't expose ourselves to emitting silently incompatible code.

The clang diagnostic is already committed in https://reviews.llvm.org/rG66225db98d832bec75ffb96298107c015b0035f0 but I should have updated the description for clarity.

ZarkoCA requested review of this revision.Sep 24 2021, 8:06 AM