This is an archive of the discontinued LLVM Phabricator instance.

[AIX] Use VSSRC/VSFRC Register classes for f32/f64 callee arguments on P8 and above
ClosedPublic

Authored by ZarkoCA on Jun 16 2021, 9:39 AM.

Details

Summary

Adding usage of VSSRC and VSFRC when adding the live in registers on AIX.
This matches the behaviour of the rest of PPC Subtargets.

Diff Detail

Unit TestsFailed

Event Timeline

ZarkoCA created this revision.Jun 16 2021, 9:39 AM
ZarkoCA requested review of this revision.Jun 16 2021, 9:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2021, 9:39 AM
jsji added a reviewer: Restricted Project.Jun 16 2021, 12:58 PM
jsji added a reviewer: qiucf.
qiucf added inline comments.Jun 16 2021, 11:10 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6741

Better to comment it if this method is only for AIX?

llvm/test/CodeGen/PowerPC/aix-p8vector-liveins.ll
5

Maybe better to pre-commit this file? (or at least generate diff after staging it)

ZarkoCA updated this revision to Diff 352797.Jun 17 2021, 11:19 AM

Added comment to clarify getRegClassforSVT() is only used by AIX.
Modified test case.

ZarkoCA marked an inline comment as done.Jun 17 2021, 11:22 AM
ZarkoCA added inline comments.
llvm/test/CodeGen/PowerPC/aix-p8vector-liveins.ll
5

I could do that if you feel strongly about it but I'd like the test case to be associated with this patch since we already have many AIX calling convention test cases. I don't think adding it alone add any value besides what this patch will show and in that case, in my opinion, it's better to have in the patch.

I did add a RUN step for running with -mcpu=pwr8 but with -power8-vector to show try and address your concern in some way. Does that work?

qiucf added inline comments.Jun 17 2021, 7:34 PM
llvm/test/CodeGen/PowerPC/aix-p8vector-liveins.ll
5

Got it. Actually this also looks good :-) since we can easily tell the difference between pwr7 and pwr8.

We may not need -mcpu=pwr8 -mattr=-power8-vector here? I think that option set should be just for testing pwr8 scheduling under pwr7 instructions only.

ZarkoCA updated this revision to Diff 353401.Jun 21 2021, 9:45 AM
  • Remove no-p8-vector RUN line
  • use Register instead of unsigned for Reg variable as per clang-tidy check

ping @qiucf Have your comments been addressed sufficiently?

qiucf added a comment.Jul 6 2021, 9:04 AM

Not sure if other reviewers have any comments, this looks fine to me.

lkail added a subscriber: lkail.Jul 6 2021, 10:08 AM
lkail added inline comments.
llvm/test/CodeGen/PowerPC/aix-p8vector-liveins.ll
2

Maybe it's missing REQUIRES:asserts or -debug-only won't work.

-stop-after=finalize-isel should work already? IIRC, machine-cp is a very late pass.

ZarkoCA updated this revision to Diff 356817.Jul 6 2021, 1:48 PM
  • Use vsfr when the target has vsx not only on P8 and above
  • use stop-after=finalize-isel instead of debug-only option
ZarkoCA marked 3 inline comments as done.Jul 6 2021, 1:50 PM
ZarkoCA added inline comments.
llvm/test/CodeGen/PowerPC/aix-p8vector-liveins.ll
2

Thank you, that is much better.

nemanjai accepted this revision.Jul 7 2021, 3:49 AM

Other than a small test case nit, LGTM.

llvm/test/CodeGen/PowerPC/aix-p8vector-liveins.ll
7

I think you should probably also have a PWR7 VSX run line since we will use VSFRC for f64 with that combination (i.e. we don't need Power8 vector in order to use VSFRC).

This revision is now accepted and ready to land.Jul 7 2021, 3:49 AM
This revision was automatically updated to reflect the committed changes.
ZarkoCA marked an inline comment as done.