Page MenuHomePhabricator

[PowerPC] Add custom lowering for SELECT_CC fp128 using xsmaxcqp
ClosedPublic

Authored by tingwang on Jan 11 2022, 3:39 AM.

Details

Summary

Power ISA 3.1 adds xsmaxcqp/xsmincqp for quad-precision type-c max/min selection, and this opens the opportunity to improve instruction selection on: llvm.maxnum.f128, llvm.minnum.f128, and select_cc ordered gt/lt and (don't care) gt/lt.

Diff Detail

Event Timeline

tingwang created this revision.Jan 11 2022, 3:39 AM
tingwang requested review of this revision.Jan 11 2022, 3:39 AM
tingwang retitled this revision from Add custom lowering for SELECT_CC fp128 using xsmaxcqp to [PowerPC] Add custom lowering for SELECT_CC fp128 using xsmaxcqp.Jan 11 2022, 3:39 AM
qiucf added a subscriber: qiucf.Jan 11 2022, 4:10 AM
qiucf added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
1288

We already have a setOperationAction(ISD::SELECT_CC, MVT::f128, Custom); under Subtarget.hasVSX(), so don't need this.

7919

We don't need to add new ISD node, but XS(MAX|MIN)CDP node need renaming.

llvm/lib/Target/PowerPC/PPCInstrInfo.td
203

Operator PPCxsmaxc is type agnostic, we don't need to define PPCxsmaxcq.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
1680–1681

I'm not sure why xscmpeqqp are located here, but this should be better placed at PPCInstrVSX.td.

llvm/test/CodeGen/PowerPC/scalar-min-max-p10.ll
2

Why not add them into scalar-min-max.ll (with a new -mcpu=pwr10 runline)?

tingwang marked 2 inline comments as done.Jan 11 2022, 5:06 PM
tingwang added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
1288

That setOperationAction is located under the else part of Subtarget.hasP9Vector(), since P9Vector is part of P9InheritableFeatures, P10 test Subtarget.hasP9Vector() as true, so not hit that setOperationAction. Since this is supported by P10, and do not see Subtarget.hasP10Vector() predicate in this file, so added one under the Subtarget.hasAltivec() big block.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
1680–1681

Thanks! I will move XS(MAX|MIN)CQP to sit together with XSMAXCDP.

llvm/test/CodeGen/PowerPC/scalar-min-max-p10.ll
2

That test checks pwr8 behavior, which I don't like to do for fp128, and that's why added a separate file.

tingwang marked an inline comment as done and 3 inline comments as not done.Jan 11 2022, 5:07 PM
tingwang added inline comments.Jan 11 2022, 5:32 PM
llvm/lib/Target/PowerPC/PPCInstrPrefix.td
1680–1681

A closer look at PPCInstrPrefix.td and PPCInstrVSX.td, all instructions with Power10Instrs predicate are located inside PPCInstrPrefix.td, and does not have any in PPCInstrVSX.td. There maybe some other preference regarding the placement, guess just leave it as is...

tingwang updated this revision to Diff 399508.Jan 12 2022, 4:32 PM

Renamed PPCISD::XS[MAX|MIN]CDP to PPCISD::XS[MAX|MIN]C, and use the nodes for both dual-precision and quad-precision type-c instructions

qiucf added inline comments.Jan 17 2022, 2:17 AM
llvm/lib/Target/PowerPC/PPCInstrPrefix.td
1680–1681

The name 'Prefix' stands for new ISA 3.1 feature - prefixed instruction, but 128-bit fp max/min is not one of them. Anyway, it's okay to leave them here and we'll do clean-up in the future when all Power10 instructions are implemented.

shchenz added inline comments.Jan 17 2022, 6:50 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
1288

What if we set Custom for SELECT_CC for all targets hasVSX()? I don't see a reason why we can not set setOperationAction(ISD::SELECT_CC, MVT::f128, Custom); on Power9. If so, we can just set SELECT_CC for type f128 under hasVSX() instead of setting it two times for P10 and hasVSX()?

tingwang added inline comments.Jan 17 2022, 8:28 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
1288

Had a try doing Custom on Power9 to SELECT_CC, that path caused dump. Following is the path taken by fp128/Power9/SELECT_CC, and picks up PPC::XSCMPUQP:
PPCDAGToDAGISel::Select
-> case ISD::SELECT_CC
--> SelectCC()

Seems the expected code paths for Power9 and Power10 are different, then we still need a predicate to differentiate that?

shchenz added inline comments.Jan 19 2022, 12:35 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
1288

OK, thanks for the explanation. I think it crashes because we don't handle PPCISD::XSMAXC and PPCISD::XSMINC for type f128 on Power9 in the legalizer.

However, I really think the logic for select_cc is kind of confusing on Power, we legalize it(set operation as custom) for Power8 and Power10, and we set it legal on Power9 and then do the selecting just before we select from td files. Maybe we can do some code refactor here later to make the logic be more clear. This should be out of this PR.

llvm/test/CodeGen/PowerPC/scalar-min-max-p10.ll
2

Better to generate check lines by using scripts like ./utils/update_llc_test_checks.py.

And could you please also add a RUN line for AIX?

nemanjai added inline comments.Jan 19 2022, 2:59 PM
llvm/lib/Target/PowerPC/PPCInstrPrefix.td
1680–1681

Please do not move any instructions out of this file piecemeal at this time. The name of this file is temporary as we kept all the Power10 instructions in one file during the incremental development process prior to the announcement of Power10.

We need to decide where we want the various groups of instructions to live and move them and/or rename files accordingly. This will be done all at once to make it obvious in the repo history when this happened and what was happening.

tingwang added inline comments.Jan 19 2022, 11:44 PM
llvm/lib/Target/PowerPC/PPCInstrPrefix.td
1680–1681

Thank you for the input. Will leave them here.

llvm/test/CodeGen/PowerPC/scalar-min-max-p10.ll
2

Thanks for the comments, will update the test case using the script.

tingwang updated this revision to Diff 401523.Jan 19 2022, 11:48 PM

(1) Add llvm.maxnum/minnum.f128 check case for P10
(2) Update test case using the script: update_llc_test_checks.py

shchenz accepted this revision as: shchenz.Jan 20 2022, 12:52 AM

LGTM. Please hold on some days for other reviewers. Thanks for adding this support.

This revision is now accepted and ready to land.Jan 20 2022, 12:52 AM
amyk accepted this revision.Jan 25 2022, 6:20 AM
amyk added a subscriber: amyk.

Overall LGTM.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
1680–1681

These are meant to be put under [IsISA3_1, HasVSX], right?

This revision was landed with ongoing or failed builds.Feb 9 2022, 6:49 PM
This revision was automatically updated to reflect the committed changes.